From 8bda2ca4324bb7f3d2ec0355718f86e96de19d2e Mon Sep 17 00:00:00 2001 From: Jon Meow <46229924+jonmeow@users.noreply.github.com> Date: Mon, 18 Oct 2021 10:25:52 -0700 Subject: [PATCH] Refactor Pattern and Member accessors. (#889) --- executable_semantics/ast/member.cpp | 2 +- executable_semantics/ast/member.h | 6 +- executable_semantics/ast/pattern.cpp | 20 +++--- executable_semantics/ast/pattern.h | 56 ++++++++--------- executable_semantics/ast/pattern_test.cpp | 14 ++--- .../interpreter/interpreter.cpp | 29 +++++---- .../interpreter/type_checker.cpp | 63 +++++++++---------- 7 files changed, 94 insertions(+), 96 deletions(-) diff --git a/executable_semantics/ast/member.cpp b/executable_semantics/ast/member.cpp index 4aa479596523..0e7fd7f7d575 100644 --- a/executable_semantics/ast/member.cpp +++ b/executable_semantics/ast/member.cpp @@ -15,7 +15,7 @@ void Member::Print(llvm::raw_ostream& out) const { switch (kind()) { case Kind::FieldMember: const auto& field = cast(*this); - out << "var " << *field.Binding() << ";\n"; + out << "var " << field.binding() << ";\n"; break; } } diff --git a/executable_semantics/ast/member.h b/executable_semantics/ast/member.h index 409bf66fa54e..c8c2a88d9989 100644 --- a/executable_semantics/ast/member.h +++ b/executable_semantics/ast/member.h @@ -54,19 +54,19 @@ class Member { class FieldMember : public Member { public: FieldMember(SourceLocation source_loc, Nonnull binding) - : Member(Kind::FieldMember, source_loc), binding(binding) {} + : Member(Kind::FieldMember, source_loc), binding_(binding) {} static auto classof(const Member* member) -> bool { return member->kind() == Kind::FieldMember; } - auto Binding() const -> Nonnull { return binding; } + auto binding() const -> const BindingPattern& { return *binding_; } private: // TODO: split this into a non-optional name and a type, initialized by // a constructor that takes a BindingPattern and handles errors like a // missing name. - Nonnull binding; + Nonnull binding_; }; } // namespace Carbon diff --git a/executable_semantics/ast/pattern.cpp b/executable_semantics/ast/pattern.cpp index 40ac58f59509..4a444b871002 100644 --- a/executable_semantics/ast/pattern.cpp +++ b/executable_semantics/ast/pattern.cpp @@ -24,19 +24,19 @@ void Pattern::Print(llvm::raw_ostream& out) const { break; case Kind::BindingPattern: { const auto& binding = cast(*this); - if (binding.Name().has_value()) { - out << *binding.Name(); + if (binding.name().has_value()) { + out << *binding.name(); } else { out << "_"; } - out << ": " << *binding.Type(); + out << ": " << binding.type(); break; } case Kind::TuplePattern: { const auto& tuple = cast(*this); out << "("; llvm::ListSeparator sep; - for (Nonnull field : tuple.Fields()) { + for (Nonnull field : tuple.fields()) { out << sep << *field; } out << ")"; @@ -44,12 +44,12 @@ void Pattern::Print(llvm::raw_ostream& out) const { } case Kind::AlternativePattern: { const auto& alternative = cast(*this); - out << *alternative.ChoiceType() << "." << alternative.AlternativeName() - << *alternative.Arguments(); + out << alternative.choice_type() << "." << alternative.alternative_name() + << alternative.arguments(); break; } case Kind::ExpressionPattern: - out << *cast(*this).Expression(); + out << cast(*this).expression(); break; } } @@ -88,9 +88,9 @@ AlternativePattern::AlternativePattern(SourceLocation source_loc, Nonnull alternative, Nonnull arguments) : Pattern(Kind::AlternativePattern, source_loc), - choice_type(&RequireFieldAccess(alternative).aggregate()), - alternative_name(RequireFieldAccess(alternative).field()), - arguments(arguments) {} + choice_type_(&RequireFieldAccess(alternative).aggregate()), + alternative_name_(RequireFieldAccess(alternative).field()), + arguments_(arguments) {} auto ParenExpressionToParenPattern(Nonnull arena, const ParenContents& contents) diff --git a/executable_semantics/ast/pattern.h b/executable_semantics/ast/pattern.h index 8a9eb736002e..90587c03bf89 100644 --- a/executable_semantics/ast/pattern.h +++ b/executable_semantics/ast/pattern.h @@ -92,42 +92,42 @@ class BindingPattern : public Pattern { BindingPattern(SourceLocation source_loc, std::optional name, Nonnull type) : Pattern(Kind::BindingPattern, source_loc), - name(std::move(name)), - type(type) {} + name_(std::move(name)), + type_(type) {} static auto classof(const Pattern* pattern) -> bool { return pattern->kind() == Kind::BindingPattern; } // The name this pattern binds, if any. - auto Name() const -> const std::optional& { return name; } + auto name() const -> const std::optional& { return name_; } // The pattern specifying the type of values that this pattern matches. - auto Type() const -> Nonnull { return type; } - auto Type() -> Nonnull { return type; } + auto type() const -> const Pattern& { return *type_; } + auto type() -> Pattern& { return *type_; } private: - std::optional name; - Nonnull type; + std::optional name_; + Nonnull type_; }; // A pattern that matches a tuple value field-wise. class TuplePattern : public Pattern { public: TuplePattern(SourceLocation source_loc, std::vector> fields) - : Pattern(Kind::TuplePattern, source_loc), fields(std::move(fields)) {} + : Pattern(Kind::TuplePattern, source_loc), fields_(std::move(fields)) {} static auto classof(const Pattern* pattern) -> bool { return pattern->kind() == Kind::TuplePattern; } - auto Fields() const -> llvm::ArrayRef> { - return fields; + auto fields() const -> llvm::ArrayRef> { + return fields_; } - auto Fields() -> llvm::ArrayRef> { return fields; } + auto fields() -> llvm::ArrayRef> { return fields_; } private: - std::vector> fields; + std::vector> fields_; }; // Converts paren_contents to a Pattern, interpreting the parentheses as @@ -161,9 +161,9 @@ class AlternativePattern : public Pattern { std::string alternative_name, Nonnull arguments) : Pattern(Kind::AlternativePattern, source_loc), - choice_type(choice_type), - alternative_name(std::move(alternative_name)), - arguments(arguments) {} + choice_type_(choice_type), + alternative_name_(std::move(alternative_name)), + arguments_(arguments) {} // Constructs an AlternativePattern that matches the alternative specified // by `alternative`, if its arguments match `arguments`. @@ -175,18 +175,18 @@ class AlternativePattern : public Pattern { return pattern->kind() == Kind::AlternativePattern; } - auto ChoiceType() const -> Nonnull { return choice_type; } - auto ChoiceType() -> Nonnull { return choice_type; } - auto AlternativeName() const -> const std::string& { - return alternative_name; + auto choice_type() const -> const Expression& { return *choice_type_; } + auto choice_type() -> Expression& { return *choice_type_; } + auto alternative_name() const -> const std::string& { + return alternative_name_; } - auto Arguments() const -> Nonnull { return arguments; } - auto Arguments() -> Nonnull { return arguments; } + auto arguments() const -> const TuplePattern& { return *arguments_; } + auto arguments() -> TuplePattern& { return *arguments_; } private: - Nonnull choice_type; - std::string alternative_name; - Nonnull arguments; + Nonnull choice_type_; + std::string alternative_name_; + Nonnull arguments_; }; // A pattern that matches a value if it is equal to the value of a given @@ -195,17 +195,17 @@ class ExpressionPattern : public Pattern { public: ExpressionPattern(Nonnull expression) : Pattern(Kind::ExpressionPattern, expression->source_loc()), - expression(expression) {} + expression_(expression) {} static auto classof(const Pattern* pattern) -> bool { return pattern->kind() == Kind::ExpressionPattern; } - auto Expression() const -> Nonnull { return expression; } - auto Expression() -> Nonnull { return expression; } + auto expression() const -> const Expression& { return *expression_; } + auto expression() -> Expression& { return *expression_; } private: - Nonnull expression; + Nonnull expression_; }; } // namespace Carbon diff --git a/executable_semantics/ast/pattern_test.cpp b/executable_semantics/ast/pattern_test.cpp index 36b9667dffed..d7b23a32deba 100644 --- a/executable_semantics/ast/pattern_test.cpp +++ b/executable_semantics/ast/pattern_test.cpp @@ -38,7 +38,7 @@ TEST_F(PatternTest, EmptyAsPattern) { PatternFromParenContents(&arena, FakeSourceLoc(1), contents); EXPECT_EQ(pattern->source_loc(), FakeSourceLoc(1)); ASSERT_TRUE(isa(*pattern)); - EXPECT_THAT(cast(*pattern).Fields(), IsEmpty()); + EXPECT_THAT(cast(*pattern).fields(), IsEmpty()); } TEST_F(PatternTest, EmptyAsTuplePattern) { @@ -47,7 +47,7 @@ TEST_F(PatternTest, EmptyAsTuplePattern) { Nonnull tuple = TuplePatternFromParenContents(&arena, FakeSourceLoc(1), contents); EXPECT_EQ(tuple->source_loc(), FakeSourceLoc(1)); - EXPECT_THAT(tuple->Fields(), IsEmpty()); + EXPECT_THAT(tuple->fields(), IsEmpty()); } TEST_F(PatternTest, UnaryNoCommaAsPattern) { @@ -75,7 +75,7 @@ TEST_F(PatternTest, UnaryNoCommaAsTuplePattern) { Nonnull tuple = TuplePatternFromParenContents(&arena, FakeSourceLoc(1), contents); EXPECT_EQ(tuple->source_loc(), FakeSourceLoc(1)); - EXPECT_THAT(tuple->Fields(), ElementsAre(AutoField())); + EXPECT_THAT(tuple->fields(), ElementsAre(AutoField())); } TEST_F(PatternTest, UnaryWithCommaAsPattern) { @@ -87,7 +87,7 @@ TEST_F(PatternTest, UnaryWithCommaAsPattern) { PatternFromParenContents(&arena, FakeSourceLoc(1), contents); EXPECT_EQ(pattern->source_loc(), FakeSourceLoc(1)); ASSERT_TRUE(isa(*pattern)); - EXPECT_THAT(cast(*pattern).Fields(), ElementsAre(AutoField())); + EXPECT_THAT(cast(*pattern).fields(), ElementsAre(AutoField())); } TEST_F(PatternTest, UnaryWithCommaAsTuplePattern) { @@ -98,7 +98,7 @@ TEST_F(PatternTest, UnaryWithCommaAsTuplePattern) { Nonnull tuple = TuplePatternFromParenContents(&arena, FakeSourceLoc(1), contents); EXPECT_EQ(tuple->source_loc(), FakeSourceLoc(1)); - EXPECT_THAT(tuple->Fields(), ElementsAre(AutoField())); + EXPECT_THAT(tuple->fields(), ElementsAre(AutoField())); } TEST_F(PatternTest, BinaryAsPattern) { @@ -111,7 +111,7 @@ TEST_F(PatternTest, BinaryAsPattern) { PatternFromParenContents(&arena, FakeSourceLoc(1), contents); EXPECT_EQ(pattern->source_loc(), FakeSourceLoc(1)); ASSERT_TRUE(isa(*pattern)); - EXPECT_THAT(cast(*pattern).Fields(), + EXPECT_THAT(cast(*pattern).fields(), ElementsAre(AutoField(), AutoField())); } @@ -124,7 +124,7 @@ TEST_F(PatternTest, BinaryAsTuplePattern) { Nonnull tuple = TuplePatternFromParenContents(&arena, FakeSourceLoc(1), contents); EXPECT_EQ(tuple->source_loc(), FakeSourceLoc(1)); - EXPECT_THAT(tuple->Fields(), ElementsAre(AutoField(), AutoField())); + EXPECT_THAT(tuple->fields(), ElementsAre(AutoField(), AutoField())); } } // namespace diff --git a/executable_semantics/interpreter/interpreter.cpp b/executable_semantics/interpreter/interpreter.cpp index 258c6f84ae32..9cbf1349ac4b 100644 --- a/executable_semantics/interpreter/interpreter.cpp +++ b/executable_semantics/interpreter/interpreter.cpp @@ -131,12 +131,11 @@ void Interpreter::InitEnv(const Declaration& d, Env* env) { for (Nonnull m : class_def.members()) { switch (m->kind()) { case Member::Kind::FieldMember: { - Nonnull binding = - cast(*m).Binding(); - Nonnull type_expression = - cast(*binding->Type()).Expression(); - auto type = InterpExp(Env(arena), type_expression); - fields.push_back(make_pair(*binding->Name(), type)); + const BindingPattern& binding = cast(*m).binding(); + const Expression& type_expression = + cast(binding.type()).expression(); + auto type = InterpExp(Env(arena), &type_expression); + fields.push_back(make_pair(*binding.name(), type)); break; } } @@ -167,7 +166,7 @@ void Interpreter::InitEnv(const Declaration& d, Env* env) { // result of evaluating the initializer. auto v = InterpExp(*env, &var.initializer()); Address a = heap.AllocateValue(v); - env->Set(*var.binding().Name(), a); + env->Set(*var.binding().name(), a); break; } } @@ -674,20 +673,20 @@ auto Interpreter::StepPattern() -> Transition { case Pattern::Kind::BindingPattern: { const auto& binding = cast(*pattern); if (act->pos() == 0) { - return Spawn{arena->New(binding.Type())}; + return Spawn{arena->New(&binding.type())}; } else { - return Done{arena->New(binding.Name(), + return Done{arena->New(binding.name(), act->results()[0])}; } } case Pattern::Kind::TuplePattern: { const auto& tuple = cast(*pattern); - if (act->pos() < static_cast(tuple.Fields().size())) { + if (act->pos() < static_cast(tuple.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} - return Spawn{arena->New(tuple.Fields()[act->pos()])}; + return Spawn{arena->New(tuple.fields()[act->pos()])}; } else { return Done{arena->New(act->results())}; } @@ -695,20 +694,20 @@ auto Interpreter::StepPattern() -> Transition { case Pattern::Kind::AlternativePattern: { const auto& alternative = cast(*pattern); if (act->pos() == 0) { - return Spawn{arena->New(alternative.ChoiceType())}; + return Spawn{arena->New(&alternative.choice_type())}; } else if (act->pos() == 1) { - return Spawn{arena->New(alternative.Arguments())}; + return Spawn{arena->New(&alternative.arguments())}; } else { CHECK(act->pos() == 2); const auto& choice_type = cast(*act->results()[0]); - return Done{arena->New(alternative.AlternativeName(), + return Done{arena->New(alternative.alternative_name(), choice_type.Name(), act->results()[1])}; } } case Pattern::Kind::ExpressionPattern: return Delegate{arena->New( - cast(*pattern).Expression())}; + &cast(*pattern).expression())}; } } diff --git a/executable_semantics/interpreter/type_checker.cpp b/executable_semantics/interpreter/type_checker.cpp index 8d63645bb0e1..788d62a0a67c 100644 --- a/executable_semantics/interpreter/type_checker.cpp +++ b/executable_semantics/interpreter/type_checker.cpp @@ -726,17 +726,17 @@ auto TypeChecker::TypeCheckPattern( } case Pattern::Kind::BindingPattern: { auto& binding = cast(*p); - TypeCheckPattern(binding.Type(), types, values, std::nullopt); + TypeCheckPattern(&binding.type(), types, values, std::nullopt); Nonnull type = - interpreter.InterpPattern(values, binding.Type()); + interpreter.InterpPattern(values, &binding.type()); if (expected) { if (IsConcreteType(type)) { ExpectType(p->source_loc(), "name binding", type, *expected); } else { std::optional values = interpreter.PatternMatch( - type, *expected, binding.Type()->source_loc()); + type, *expected, binding.type().source_loc()); if (values == std::nullopt) { - FATAL_COMPILATION_ERROR(binding.Type()->source_loc()) + FATAL_COMPILATION_ERROR(binding.type().source_loc()) << "Type pattern '" << *type << "' does not match actual type '" << **expected << "'"; } @@ -746,8 +746,8 @@ auto TypeChecker::TypeCheckPattern( } } ExpectIsConcreteType(binding.source_loc(), type); - if (binding.Name().has_value()) { - types.Set(*binding.Name(), type); + if (binding.name().has_value()) { + types.Set(*binding.name(), type); } SetStaticType(&binding, type); return TCResult(types); @@ -759,13 +759,13 @@ auto TypeChecker::TypeCheckPattern( if (expected && (*expected)->kind() != Value::Kind::TupleValue) { FATAL_COMPILATION_ERROR(p->source_loc()) << "didn't expect a tuple"; } - if (expected && tuple.Fields().size() != + if (expected && tuple.fields().size() != cast(**expected).Elements().size()) { FATAL_COMPILATION_ERROR(tuple.source_loc()) << "tuples of different length"; } - for (size_t i = 0; i < tuple.Fields().size(); ++i) { - Nonnull field = tuple.Fields()[i]; + for (size_t i = 0; i < tuple.fields().size(); ++i) { + Nonnull field = tuple.fields()[i]; std::optional> expected_field_type; if (expected) { expected_field_type = cast(**expected).Elements()[i]; @@ -781,7 +781,7 @@ auto TypeChecker::TypeCheckPattern( case Pattern::Kind::AlternativePattern: { auto& alternative = cast(*p); Nonnull choice_type = - interpreter.InterpExp(values, alternative.ChoiceType()); + interpreter.InterpExp(values, &alternative.choice_type()); if (choice_type->kind() != Value::Kind::ChoiceType) { FATAL_COMPILATION_ERROR(alternative.source_loc()) << "alternative pattern does not name a choice type."; @@ -791,22 +791,22 @@ auto TypeChecker::TypeCheckPattern( *expected, choice_type); } std::optional> parameter_types = - FindInVarValues(alternative.AlternativeName(), + FindInVarValues(alternative.alternative_name(), cast(*choice_type).Alternatives()); if (parameter_types == std::nullopt) { FATAL_COMPILATION_ERROR(alternative.source_loc()) - << "'" << alternative.AlternativeName() + << "'" << alternative.alternative_name() << "' is not an alternative of " << *choice_type; } - TCResult arg_results = TypeCheckPattern(alternative.Arguments(), types, + TCResult arg_results = TypeCheckPattern(&alternative.arguments(), types, values, *parameter_types); SetStaticType(&alternative, choice_type); return TCResult(arg_results.types); } case Pattern::Kind::ExpressionPattern: { - const auto& expression = cast(*p).Expression(); - TCResult result = TypeCheckExp(expression, types, values); - SetStaticType(p, &expression->static_type()); + auto& expression = cast(*p).expression(); + TCResult result = TypeCheckExp(&expression, types, values); + SetStaticType(p, &expression.static_type()); return TCResult(result.types); } } @@ -1097,19 +1097,18 @@ auto TypeChecker::TypeOfClassDef(const ClassDefinition* sd, TypeEnv /*types*/, for (Nonnull m : sd->members()) { switch (m->kind()) { case Member::Kind::FieldMember: { - Nonnull binding = - cast(*m).Binding(); - if (!binding->Name().has_value()) { - FATAL_COMPILATION_ERROR(binding->source_loc()) + const BindingPattern& binding = cast(*m).binding(); + if (!binding.name().has_value()) { + FATAL_COMPILATION_ERROR(binding.source_loc()) << "Struct members must have names"; } - const auto* binding_type = dyn_cast(binding->Type()); + const auto* binding_type = dyn_cast(&binding.type()); if (binding_type == nullptr) { - FATAL_COMPILATION_ERROR(binding->source_loc()) + FATAL_COMPILATION_ERROR(binding.source_loc()) << "Struct members must have explicit types"; } - auto type = interpreter.InterpExp(ct_top, binding_type->Expression()); - fields.push_back(std::make_pair(*binding->Name(), type)); + auto type = interpreter.InterpExp(ct_top, &binding_type->expression()); + fields.push_back(std::make_pair(*binding.name(), type)); break; } } @@ -1128,11 +1127,11 @@ static auto GetName(const Declaration& d) -> const std::string& { return cast(d).name(); case Declaration::Kind::VariableDeclaration: { const BindingPattern& binding = cast(d).binding(); - if (!binding.Name().has_value()) { + if (!binding.name().has_value()) { FATAL_COMPILATION_ERROR(binding.source_loc()) << "Top-level variable declarations must have names"; } - return *binding.Name(); + return *binding.name(); } } } @@ -1159,14 +1158,14 @@ void TypeChecker::TypeCheck(Nonnull d, const TypeEnv& types, // declaration with annotated types. TypeCheckExp(&var.initializer(), types, values); const auto* binding_type = - dyn_cast(var.binding().Type()); + dyn_cast(&var.binding().type()); if (binding_type == nullptr) { // TODO: consider adding support for `auto` FATAL_COMPILATION_ERROR(var.source_loc()) << "Type of a top-level variable must be an expression."; } Nonnull declared_type = - interpreter.InterpExp(values, binding_type->Expression()); + interpreter.InterpExp(values, &binding_type->expression()); ExpectType(var.source_loc(), "initializer of variable", declared_type, &var.initializer().static_type()); return; @@ -1212,11 +1211,11 @@ void TypeChecker::TopLevel(Nonnull d, TypeCheckContext* tops) { auto& var = cast(*d); // Associate the variable name with it's declared type in the // compile-time symbol table. - Nonnull type = - cast(*var.binding().Type()).Expression(); + Expression& type = + cast(var.binding().type()).expression(); Nonnull declared_type = - interpreter.InterpExp(tops->values, type); - tops->types.Set(*var.binding().Name(), declared_type); + interpreter.InterpExp(tops->values, &type); + tops->types.Set(*var.binding().name(), declared_type); break; } }