From 59ff7743c097d279b2fbd806b8006f1b86abde61 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Tue, 14 Feb 2023 13:28:52 -0800 Subject: [PATCH] Expect parentheses after an alternative only if they were present in its declaration (#2605) The design of choice types expects the declaration of an alternative to match the usage: if an alternative is declared as `None`, then it should be used as `None` not `None()`, and if it is declared as `None()` then it should be used as `None()` not `None`. Update explorer to match. Also clean up the handling of choice types and alternative values a little in general, by moving away from identifying choice types and alternatives as strings and towards identifying them symbolically. Closes #2422 --- common/fuzzing/proto_to_carbon.cpp | 4 +- explorer/ast/declaration.cpp | 15 ++- explorer/ast/declaration.h | 34 +++++-- explorer/data/prelude.carbon | 40 ++++---- explorer/fuzzing/ast_to_proto.cpp | 6 +- explorer/interpreter/interpreter.cpp | 12 ++- explorer/interpreter/pattern_analysis.cpp | 13 ++- explorer/interpreter/resolve_names.cpp | 5 +- explorer/interpreter/type_checker.cpp | 95 ++++++++++++------- explorer/interpreter/value.cpp | 51 ++++++---- explorer/interpreter/value.h | 78 +++++++-------- explorer/syntax/parser.ypp | 5 +- explorer/testdata/basic_syntax/choice.carbon | 40 ++++---- .../fail_choice_extra_parens.carbon | 23 +++++ .../basic_syntax/fail_choice_no_parens.carbon | 10 +- .../fail_choice_pattern_extra_parens.carbon | 23 +++++ .../fail_choice_pattern_no_parens.carbon | 25 +++++ .../testdata/let/fail_match_choice.carbon | 4 +- 18 files changed, 315 insertions(+), 168 deletions(-) create mode 100644 explorer/testdata/basic_syntax/fail_choice_extra_parens.carbon create mode 100644 explorer/testdata/basic_syntax/fail_choice_pattern_extra_parens.carbon create mode 100644 explorer/testdata/basic_syntax/fail_choice_pattern_no_parens.carbon diff --git a/common/fuzzing/proto_to_carbon.cpp b/common/fuzzing/proto_to_carbon.cpp index 74299b55eef9..b8b84aebf87c 100644 --- a/common/fuzzing/proto_to_carbon.cpp +++ b/common/fuzzing/proto_to_carbon.cpp @@ -853,7 +853,9 @@ static auto DeclarationToCarbon(const Fuzzing::Declaration& declaration, for (const auto& alternative : choice.alternatives()) { out << sep; IdentifierToCarbon(alternative.name(), out); - TupleLiteralExpressionToCarbon(alternative.signature(), out); + if (alternative.has_signature()) { + TupleLiteralExpressionToCarbon(alternative.signature(), out); + } } out << "}"; break; diff --git a/explorer/ast/declaration.cpp b/explorer/ast/declaration.cpp index 876b284d34eb..6c2d0448e27d 100644 --- a/explorer/ast/declaration.cpp +++ b/explorer/ast/declaration.cpp @@ -429,11 +429,24 @@ auto ImplDeclaration::Create(Nonnull arena, SourceLocation source_loc, } void AlternativeSignature::Print(llvm::raw_ostream& out) const { - out << "alt " << name() << " " << signature(); + out << "alt " << name(); + if (auto sig = signature()) { + out << **signature(); + } } void AlternativeSignature::PrintID(llvm::raw_ostream& out) const { out << name(); } +auto ChoiceDeclaration::FindAlternative(std::string_view name) const + -> std::optional { + for (auto* alt : alternatives()) { + if (alt->name() == name) { + return alt; + } + } + return std::nullopt; +} + } // namespace Carbon diff --git a/explorer/ast/declaration.h b/explorer/ast/declaration.h index 3e2f16307a38..88cceb31e667 100644 --- a/explorer/ast/declaration.h +++ b/explorer/ast/declaration.h @@ -468,7 +468,7 @@ class MixDeclaration : public Declaration { class AlternativeSignature : public AstNode { public: AlternativeSignature(SourceLocation source_loc, std::string name, - Nonnull signature) + std::optional> signature) : AstNode(AstNodeKind::AlternativeSignature, source_loc), name_(std::move(name)), signature_(signature) {} @@ -481,12 +481,29 @@ class AlternativeSignature : public AstNode { } auto name() const -> const std::string& { return name_; } - auto signature() const -> const TupleLiteral& { return *signature_; } - auto signature() -> TupleLiteral& { return *signature_; } + auto signature() const -> std::optional> { + return signature_; + } + auto signature() -> std::optional> { + return signature_; + } + + // The static type signature, if any. Cannot be called before type checking. + auto static_type() const -> std::optional> { + return static_type_; + } + + // Sets the static type of the declared entity. Can only be called once, + // during typechecking. + void set_static_type(Nonnull type) { + CARBON_CHECK(!static_type_.has_value()); + static_type_ = type; + } private: std::string name_; - Nonnull signature_; + std::optional> signature_; + std::optional> static_type_; }; class ChoiceDeclaration : public Declaration { @@ -522,18 +539,15 @@ class ChoiceDeclaration : public Declaration { return alternatives_; } - void set_members(const std::vector& members) { - members_ = members; - } - auto members() const -> std::vector { return members_; } - auto value_category() const -> ValueCategory { return ValueCategory::Let; } + auto FindAlternative(std::string_view name) const + -> std::optional; + private: DeclaredName name_; std::optional> type_params_; std::vector> alternatives_; - std::vector members_; }; // Global variable definition implements the Declaration concept. diff --git a/explorer/data/prelude.carbon b/explorer/data/prelude.carbon index a18ae4a25dda..75e0f66baaae 100644 --- a/explorer/data/prelude.carbon +++ b/explorer/data/prelude.carbon @@ -181,15 +181,15 @@ impl i32 as CompareWith(Self) { fn Compare[self: Self](other: Self) -> Ordering { var comp: i32 = __intrinsic_int_compare(self, other); if (comp == -1) { - return Ordering.Less(); + return Ordering.Less; } if (comp == 0) { - return Ordering.Equivalent(); + return Ordering.Equivalent; } if (comp == 1) { - return Ordering.Greater(); + return Ordering.Greater; } - return Ordering.Incomparable(); + return Ordering.Incomparable; } } @@ -198,15 +198,15 @@ impl String as CompareWith(Self) { fn Compare[self: Self](other: Self) -> Ordering { var comp: i32 = __intrinsic_str_compare(self, other); if (comp == -1) { - return Ordering.Less(); + return Ordering.Less; } if (comp == 0) { - return Ordering.Equivalent(); + return Ordering.Equivalent; } if (comp == 1) { - return Ordering.Greater(); + return Ordering.Greater; } - return Ordering.Incomparable(); + return Ordering.Incomparable; } } @@ -230,7 +230,7 @@ impl i32 as LessWith(Self) { fn Less[self: Self](other: Self) -> bool { var comp: Ordering = self.(CompareWith(i32).Compare)(other); match (comp) { - case Ordering.Less() => { + case Ordering.Less => { return true; } } @@ -242,7 +242,7 @@ impl String as LessWith(Self) { fn Less[self: Self](other: Self) -> bool { var comp: Ordering = self.(CompareWith(String).Compare)(other); match(comp){ - case Ordering.Less() => { + case Ordering.Less => { return true; } } @@ -254,10 +254,10 @@ impl i32 as LessEqWith(Self) { fn LessEq[self: Self](other: Self) -> bool { var comp: Ordering = self.(CompareWith(i32).Compare)(other); match(comp){ - case Ordering.Less() => { + case Ordering.Less => { return true; } - case Ordering.Equivalent() => { + case Ordering.Equivalent => { return true; } } @@ -269,10 +269,10 @@ impl String as LessEqWith(Self) { fn LessEq[self: Self](other: Self) -> bool { var comp: Ordering = self.(CompareWith(String).Compare)(other); match(comp){ - case Ordering.Less() => { + case Ordering.Less => { return true; } - case Ordering.Equivalent() => { + case Ordering.Equivalent => { return true; } } @@ -284,7 +284,7 @@ impl i32 as GreaterWith(Self) { fn Greater[self: Self](other: Self) -> bool { var comp: Ordering = self.(CompareWith(i32).Compare)(other); match(comp){ - case Ordering.Greater() => { + case Ordering.Greater => { return true; } } @@ -296,7 +296,7 @@ impl String as GreaterWith(Self) { fn Greater[self: Self](other: Self) -> bool { var comp: Ordering = self.(CompareWith(String).Compare)(other); match(comp){ - case Ordering.Greater() => { + case Ordering.Greater => { return true; } } @@ -308,10 +308,10 @@ impl i32 as GreaterEqWith(Self) { fn GreaterEq[self: Self](other: Self) -> bool { var comp: Ordering = self.(CompareWith(i32).Compare)(other); match(comp){ - case Ordering.Greater() => { + case Ordering.Greater => { return true; } - case Ordering.Equivalent() => { + case Ordering.Equivalent => { return true; } } @@ -323,10 +323,10 @@ impl String as GreaterEqWith(Self) { fn GreaterEq[self: Self](other: Self) -> bool { var comp: Ordering = self.(CompareWith(String).Compare)(other); match(comp){ - case Ordering.Greater() => { + case Ordering.Greater => { return true; } - case Ordering.Equivalent() => { + case Ordering.Equivalent => { return true; } } diff --git a/explorer/fuzzing/ast_to_proto.cpp b/explorer/fuzzing/ast_to_proto.cpp index 3a281f49b9aa..2f84f2447563 100644 --- a/explorer/fuzzing/ast_to_proto.cpp +++ b/explorer/fuzzing/ast_to_proto.cpp @@ -750,8 +750,10 @@ static auto DeclarationToProto(const Declaration& declaration) choice.alternatives()) { auto* alternative_proto = choice_proto->add_alternatives(); alternative_proto->set_name(alternative->name()); - *alternative_proto->mutable_signature() = - TupleLiteralExpressionToProto(alternative->signature()); + if (auto sig = alternative->signature()) { + *alternative_proto->mutable_signature() = + TupleLiteralExpressionToProto(**sig); + } } break; } diff --git a/explorer/interpreter/interpreter.cpp b/explorer/interpreter/interpreter.cpp index 2ff179118a98..e3a58d0ad0b7 100644 --- a/explorer/interpreter/interpreter.cpp +++ b/explorer/interpreter/interpreter.cpp @@ -350,11 +350,15 @@ auto PatternMatch(Nonnull p, Nonnull v, case Value::Kind::AlternativeValue: { const auto& p_alt = cast(*p); const auto& v_alt = cast(*v); - if (p_alt.choice_name() != v_alt.choice_name() || - p_alt.alt_name() != v_alt.alt_name()) { + if (&p_alt.alternative() != &v_alt.alternative()) { return false; } - return PatternMatch(&p_alt.argument(), &v_alt.argument(), source_loc, + CARBON_CHECK(p_alt.argument().has_value() == + v_alt.argument().has_value()); + if (!p_alt.argument().has_value()) { + return true; + } + return PatternMatch(*p_alt.argument(), *v_alt.argument(), source_loc, bindings, generic_args, trace_stream, arena); } default: @@ -960,7 +964,7 @@ auto Interpreter::CallFunction(const CallExpression& call, case Value::Kind::AlternativeConstructorValue: { const auto& alt = cast(*fun); return todo_.FinishAction(arena_->New( - alt.alt_name(), alt.choice_name(), arg)); + &alt.choice(), &alt.alternative(), cast(arg))); } case Value::Kind::FunctionValue: { const auto& fun_val = cast(*fun); diff --git a/explorer/interpreter/pattern_analysis.cpp b/explorer/interpreter/pattern_analysis.cpp index 0bfb058e2d68..464df1325dfe 100644 --- a/explorer/interpreter/pattern_analysis.cpp +++ b/explorer/interpreter/pattern_analysis.cpp @@ -34,7 +34,7 @@ auto AbstractPattern::discriminator() const -> std::string_view { } } else if (const auto* value = value_.dyn_cast()) { if (const auto* alt = dyn_cast(value)) { - return alt->alt_name(); + return alt->alternative().name(); } else if (const auto* bool_val = dyn_cast(value)) { return bool_val->value() ? "true" : "false"; } @@ -47,13 +47,16 @@ auto AbstractPattern::elements_size() const -> int { if (const auto* tuple_pattern = dyn_cast(pattern)) { return tuple_pattern->fields().size(); } else if (isa(pattern)) { + // Note, AlternativePattern is only used for a pattern with arguments. An + // alternative pattern without arguments is represented as an + // AlternativeValue. return 1; } } else if (const auto* value = value_.dyn_cast()) { if (const auto* tuple = dyn_cast(value)) { return tuple->elements().size(); } else if (const auto* alt = dyn_cast(value)) { - return 1; + return alt->argument() ? 1 : 0; } } return 0; @@ -78,9 +81,9 @@ void AbstractPattern::AppendElementsTo( AbstractPattern(tuple->elements()[i], tuple_type->elements()[i])); } } else if (const auto* alt = dyn_cast(value)) { - out.push_back(AbstractPattern( - &alt->argument(), - *cast(type_)->FindAlternative(alt->alt_name()))); + if (auto arg = alt->argument()) { + out.push_back(AbstractPattern(*arg, *alt->alternative().static_type())); + } } } } diff --git a/explorer/interpreter/resolve_names.cpp b/explorer/interpreter/resolve_names.cpp index 15e5407ab3f7..095f5f3a133b 100644 --- a/explorer/interpreter/resolve_names.cpp +++ b/explorer/interpreter/resolve_names.cpp @@ -792,8 +792,9 @@ auto NameResolver::ResolveNames(Declaration& declaration, // need to check for duplicates. std::set alternative_names; for (Nonnull alternative : choice.alternatives()) { - CARBON_RETURN_IF_ERROR( - ResolveNames(alternative->signature(), choice_scope)); + if (auto sig = alternative->signature()) { + CARBON_RETURN_IF_ERROR(ResolveNames(**sig, choice_scope)); + } if (!alternative_names.insert(alternative->name()).second) { return ProgramError(alternative->source_loc()) << "Duplicate name `" << alternative->name() diff --git a/explorer/interpreter/type_checker.cpp b/explorer/interpreter/type_checker.cpp index a245bb8c8eeb..bc4cfcc4fb1d 100644 --- a/explorer/interpreter/type_checker.cpp +++ b/explorer/interpreter/type_checker.cpp @@ -2784,22 +2784,29 @@ auto TypeChecker::TypeCheckExp(Nonnull e, } case Value::Kind::ChoiceType: { const auto& choice = cast(*type); - std::optional> parameter_types = - choice.FindAlternative(access.member_name()); - if (!parameter_types.has_value()) { + std::optional> signature = + choice.declaration().FindAlternative(access.member_name()); + if (!signature.has_value()) { return ProgramError(e->source_loc()) - << "choice " << choice.name() - << " does not have an alternative named " + << choice << " does not have an alternative named " << access.member_name(); } - Nonnull substituted_parameter_type = - *parameter_types; - if (choice.IsParameterized()) { - substituted_parameter_type = - Substitute(choice.bindings(), *parameter_types); + + // If we find an alternative with no declared signature, we are + // constructing an unparameterized alternative value. + if (!(*signature)->static_type()) { + access.set_member( + arena_->New(arena_->New( + NamedValue{access.member_name(), &choice}))); + access.set_static_type(&choice); + access.set_value_category(ValueCategory::Let); + return Success(); } - Nonnull type = arena_->New( - substituted_parameter_type, &choice); + + Nonnull parameter_type = + Substitute(choice.bindings(), *(*signature)->static_type()); + Nonnull type = + arena_->New(parameter_type, &choice); // TODO: Should there be a Declaration corresponding to each // choice type alternative? access.set_member( @@ -3311,6 +3318,24 @@ auto TypeChecker::TypeCheckExp(Nonnull e, call.set_value_category(ValueCategory::Let); return Success(); } + case Value::Kind::ChoiceType: { + // Give a better diagnostic for an attempt to call a choice constant. + auto* member_access = + dyn_cast(&call.function()); + if (member_access && + isa(member_access->object().static_type())) { + CARBON_ASSIGN_OR_RETURN( + Nonnull type, + InterpExp(&member_access->object(), arena_, trace_stream_)); + if (isa(type)) { + return ProgramError(e->source_loc()) + << "alternative `" << *type << "." + << member_access->member_name() + << "` does not expect an argument list"; + } + } + [[fallthrough]]; + } default: { return ProgramError(e->source_loc()) << "in call `" << *e @@ -3995,26 +4020,30 @@ auto TypeChecker::TypeCheckPattern( "alternative pattern", &choice_type, *expected, impl_scope)); } - std::optional> parameter_types = - choice_type.FindAlternative(alternative.alternative_name()); - if (parameter_types == std::nullopt) { + std::optional> signature = + choice_type.declaration().FindAlternative( + alternative.alternative_name()); + if (!signature) { return ProgramError(alternative.source_loc()) - << "'" << alternative.alternative_name() - << "' is not an alternative of " << choice_type; + << "`" << alternative.alternative_name() + << "` is not an alternative of " << choice_type; + } + if (!(*signature)->static_type()) { + return ProgramError(alternative.source_loc()) + << "alternative `" << choice_type << "." + << alternative.alternative_name() + << "` does not expect an argument list"; } - Nonnull substituted_parameter_type = *parameter_types; - if (choice_type.IsParameterized()) { - substituted_parameter_type = - Substitute(choice_type.bindings(), *parameter_types); - } - CARBON_RETURN_IF_ERROR( - TypeCheckPattern(&alternative.arguments(), substituted_parameter_type, - impl_scope, enclosing_value_category)); + Nonnull parameter_type = + Substitute(choice_type.bindings(), *(*signature)->static_type()); + CARBON_RETURN_IF_ERROR(TypeCheckPattern(&alternative.arguments(), + parameter_type, impl_scope, + enclosing_value_category)); alternative.set_static_type(&choice_type); alternative.set_value(arena_->New( - alternative.alternative_name(), choice_type.name(), - &alternative.arguments().value())); + &choice_type, *signature, + cast(&alternative.arguments().value()))); return Success(); } case PatternKind::ExpressionPattern: { @@ -5468,14 +5497,14 @@ auto TypeChecker::DeclareChoiceDeclaration(Nonnull choice, } } - std::vector alternatives; for (Nonnull alternative : choice->alternatives()) { - CARBON_ASSIGN_OR_RETURN(auto signature, - TypeCheckTypeExp(&alternative->signature(), - *scope_info.innermost_scope)); - alternatives.push_back({alternative->name(), signature}); + if (auto signature = alternative->signature()) { + CARBON_ASSIGN_OR_RETURN( + auto type, TypeCheckTypeExp(*signature, *scope_info.innermost_scope)); + alternative->set_static_type(type); + } } - choice->set_members(alternatives); + if (choice->type_params().has_value()) { Nonnull param_name = arena_->New(choice, *choice->type_params()); diff --git a/explorer/interpreter/value.cpp b/explorer/interpreter/value.cpp index 686f186ca87c..d8555ca2bcd5 100644 --- a/explorer/interpreter/value.cpp +++ b/explorer/interpreter/value.cpp @@ -195,11 +195,15 @@ static auto GetNamedElement(Nonnull arena, Nonnull v, } case Value::Kind::ChoiceType: { const auto& choice = cast(*v); - if (!choice.FindAlternative(f)) { + auto alt = choice.declaration().FindAlternative(f); + if (!alt) { return ProgramError(source_loc) << "alternative " << f << " not in " << *v; } - return arena->New(f, choice.name()); + if ((*alt)->signature()) { + return arena->New(&choice, *alt); + } + return arena->New(&choice, *alt, std::nullopt); } case Value::Kind::NominalClassType: { // Access a class function. @@ -360,7 +364,8 @@ void Value::Print(llvm::raw_ostream& out) const { switch (kind()) { case Value::Kind::AlternativeConstructorValue: { const auto& alt = cast(*this); - out << alt.choice_name() << "." << alt.alt_name(); + out << alt.choice().declaration().name() << "." + << alt.alternative().name(); break; } case Value::Kind::BindingPlaceholderValue: { @@ -381,8 +386,11 @@ void Value::Print(llvm::raw_ostream& out) const { } case Value::Kind::AlternativeValue: { const auto& alt = cast(*this); - out << "alt " << alt.choice_name() << "." << alt.alt_name() << " " - << alt.argument(); + out << alt.choice().declaration().name() << "." + << alt.alternative().name(); + if (auto arg = alt.argument()) { + out << **arg; + } break; } case Value::Kind::StructValue: { @@ -536,6 +544,13 @@ void Value::Print(llvm::raw_ostream& out) const { } break; } + case Value::Kind::ChoiceType: { + const auto& choice_type = cast(*this); + out << "choice "; + PrintNameWithBindings(out, &choice_type.declaration(), + choice_type.type_args()); + break; + } case Value::Kind::MixinPseudoType: { const auto& mixin_type = cast(*this); out << "mixin "; @@ -647,9 +662,6 @@ void Value::Print(llvm::raw_ostream& out) const { } break; } - case Value::Kind::ChoiceType: - out << "choice " << cast(*this).name(); - break; case Value::Kind::VariableType: out << cast(*this).binding().name(); break; @@ -1013,6 +1025,17 @@ auto ValueStructurallyEqual( } return true; } + case Value::Kind::AlternativeValue: { + const auto& alt1 = cast(*v1); + const auto& alt2 = cast(*v2); + if (!TypeEqual(&alt1.choice(), &alt2.choice(), equality_ctx) || + &alt1.alternative() != &alt2.alternative()) { + return false; + } + CARBON_CHECK(alt1.argument().has_value() == alt2.argument().has_value()); + return !alt1.argument().has_value() || + ValueEqual(*alt1.argument(), *alt2.argument(), equality_ctx); + } case Value::Kind::StringValue: return cast(*v1).value() == cast(*v2).value(); case Value::Kind::ParameterizedEntityName: { @@ -1059,7 +1082,6 @@ auto ValueStructurallyEqual( case Value::Kind::StaticArrayType: return TypeEqual(v1, v2, equality_ctx); case Value::Kind::NominalClassValue: - case Value::Kind::AlternativeValue: case Value::Kind::BindingPlaceholderValue: case Value::Kind::AddrValue: case Value::Kind::AlternativeConstructorValue: @@ -1151,17 +1173,6 @@ auto ConstraintType::VisitEqualValues( return true; } -auto ChoiceType::FindAlternative(std::string_view name) const - -> std::optional> { - std::vector alternatives = declaration_->members(); - for (const NamedValue& alternative : alternatives) { - if (alternative.name == name) { - return alternative.value; - } - } - return std::nullopt; -} - auto FindFunction(std::string_view name, llvm::ArrayRef> members) -> std::optional> { diff --git a/explorer/interpreter/value.h b/explorer/interpreter/value.h index 662583aa0c39..a4f0113b94a4 100644 --- a/explorer/interpreter/value.h +++ b/explorer/interpreter/value.h @@ -26,6 +26,8 @@ namespace Carbon { class Action; class AssociatedConstant; +class ChoiceType; +class TupleValue; // A trait type that describes how to allocate an instance of `T` in an arena. // Returns the created object, which is not required to be of type `T`. @@ -379,16 +381,30 @@ class NominalClassValue : public Value { Nonnull class_value_ptr_; }; -// An alternative constructor value. -// TODO: The representation here is inappropriate: at least the choice type -// should be identified symbolically rather than by name. -class AlternativeConstructorValue : public Value { +// Common implementation of alternative values and alternative constructors. +class AlternativeValueBase : public Value { public: - AlternativeConstructorValue(std::string_view alt_name, - std::string_view choice_name) - : Value(Kind::AlternativeConstructorValue), - alt_name_(alt_name), - choice_name_(choice_name) {} + AlternativeValueBase(Kind kind, Nonnull choice, + Nonnull alternative) + : Value(kind), choice_(choice), alternative_(alternative) {} + + auto choice() const -> const ChoiceType& { return *choice_; } + auto alternative() const -> const AlternativeSignature& { + return *alternative_; + } + + private: + Nonnull choice_; + Nonnull alternative_; +}; + +// An alternative constructor value. +class AlternativeConstructorValue : public AlternativeValueBase { + public: + AlternativeConstructorValue(Nonnull choice, + Nonnull alternative) + : AlternativeValueBase(Kind::AlternativeConstructorValue, choice, + alternative) {} static auto classof(const Value* value) -> bool { return value->kind() == Kind::AlternativeConstructorValue; @@ -396,27 +412,17 @@ class AlternativeConstructorValue : public Value { template auto Decompose(F f) const { - return f(alt_name_, choice_name_); + return f(&choice(), &alternative()); } - - auto alt_name() const -> const std::string& { return alt_name_; } - auto choice_name() const -> const std::string& { return choice_name_; } - - private: - std::string alt_name_; - std::string choice_name_; }; // An alternative value. -// TODO: The representation here is inappropriate: at least the choice type -// should be identified symbolically rather than by name. -class AlternativeValue : public Value { +class AlternativeValue : public AlternativeValueBase { public: - AlternativeValue(std::string_view alt_name, std::string_view choice_name, - Nonnull argument) - : Value(Kind::AlternativeValue), - alt_name_(alt_name), - choice_name_(choice_name), + AlternativeValue(Nonnull choice, + Nonnull alternative, + std::optional> argument) + : AlternativeValueBase(Kind::AlternativeValue, choice, alternative), argument_(argument) {} static auto classof(const Value* value) -> bool { @@ -425,17 +431,15 @@ class AlternativeValue : public Value { template auto Decompose(F f) const { - return f(alt_name_, choice_name_, argument_); + return f(&choice(), &alternative(), argument_); } - auto alt_name() const -> const std::string& { return alt_name_; } - auto choice_name() const -> const std::string& { return choice_name_; } - auto argument() const -> const Value& { return *argument_; } + auto argument() const -> std::optional> { + return argument_; + } private: - std::string alt_name_; - std::string choice_name_; - Nonnull argument_; + std::optional> argument_; }; // Base class for tuple types and tuple values. These are the same other than @@ -1254,16 +1258,6 @@ class ChoiceType : public Value { return f(declaration_, bindings_); } - // TODO: Remove this. - auto name() const -> std::string_view { - return declaration_->name().inner_name(); - } - - // Returns the parameter types of the alternative with the given name, - // or nullopt if no such alternative is present. - auto FindAlternative(std::string_view name) const - -> std::optional>; - auto bindings() const -> const Bindings& { return *bindings_; } auto type_args() const -> const BindingMap& { return bindings_->args(); } diff --git a/explorer/syntax/parser.ypp b/explorer/syntax/parser.ypp index 4fe77f8af2a0..3c4311f4519b 100644 --- a/explorer/syntax/parser.ypp +++ b/explorer/syntax/parser.ypp @@ -1181,9 +1181,8 @@ alternative: { $$ = arena->New(context.source_loc(), $1, $2); } | identifier { - $$ = arena->New( - context.source_loc(), $1, - arena->New(context.source_loc())); + $$ = arena->New(context.source_loc(), $1, + std::nullopt); } ; alternative_list: diff --git a/explorer/testdata/basic_syntax/choice.carbon b/explorer/testdata/basic_syntax/choice.carbon index 2d5c0cad3a3e..0980dae66852 100644 --- a/explorer/testdata/basic_syntax/choice.carbon +++ b/explorer/testdata/basic_syntax/choice.carbon @@ -5,7 +5,7 @@ // AUTOUPDATE // RUN: %{explorer-run} // RUN: %{explorer-run-trace} -// CHECK:STDOUT: result: 0 +// CHECK:STDOUT: result: 1 package ExplorerTest api; @@ -15,26 +15,30 @@ choice Ints { Two(i32, i32) } -fn Main() -> i32 { - var x: Ints = Ints.None(); - var y: Ints = Ints.One(42); - var n: i32 = 0; - match (y) { - case Ints.None() => { n = n + 2; } - case Ints.One(x: auto) => { n = x + 1 - 42; } - case Ints.Two(a: auto, b: auto) => { n = 2; } - } - match (x) { - case Ints.One(x: auto) => { n = x + 2; } - case Ints.None() => { n = n - 1; } - case Ints.Two(x: auto, y: auto) => { n = 5; } - } - return n; -} - // Test some alternate syntaxes choice MoreInts { None(), One(i32), Two(i32, i32), } + +fn Main() -> i32 { + var x: Ints = Ints.None; + var y: Ints = Ints.One(42); + var z: MoreInts = MoreInts.None(); + var n: i32 = 0; + match (y) { + case Ints.None => { n = n + 2; } + case Ints.One(x: auto) => { n = x + 1 - 42; } + case Ints.Two(a: auto, b: auto) => { n = 2; } + } + match (x) { + case Ints.One(x: auto) => { n = x + 2; } + case Ints.None => { n = n - 1; } + case Ints.Two(x: auto, y: auto) => { n = 5; } + } + match (z) { + case MoreInts.None() => { ++n; } + } + return n; +} diff --git a/explorer/testdata/basic_syntax/fail_choice_extra_parens.carbon b/explorer/testdata/basic_syntax/fail_choice_extra_parens.carbon new file mode 100644 index 000000000000..bd9db3f314d7 --- /dev/null +++ b/explorer/testdata/basic_syntax/fail_choice_extra_parens.carbon @@ -0,0 +1,23 @@ +// 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 +// +// AUTOUPDATE +// RUN: %{not} %{explorer-run} +// RUN: %{not} %{explorer-run-trace} + +package ExplorerTest api; + +choice Ints { + None, + One(i32), + Two(i32, i32) +} + +fn Main() -> i32 { + // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/basic_syntax/fail_choice_extra_parens.carbon:[[@LINE+1]]: alternative `choice Ints.None` does not expect an argument list + match (Ints.None()) { + case Ints.None => { return 0; } + default => { return 1; } + } +} diff --git a/explorer/testdata/basic_syntax/fail_choice_no_parens.carbon b/explorer/testdata/basic_syntax/fail_choice_no_parens.carbon index 9989e4117453..7ab87b3b684e 100644 --- a/explorer/testdata/basic_syntax/fail_choice_no_parens.carbon +++ b/explorer/testdata/basic_syntax/fail_choice_no_parens.carbon @@ -9,17 +9,17 @@ package ExplorerTest api; choice Ints { - None, + None(), One(i32), Two(i32, i32) } fn Main() -> i32 { - match (Ints.None()) { - case Ints.None => { return 0; } + match (Ints.None) { + case Ints.None() => { return 0; } // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/basic_syntax/fail_choice_no_parens.carbon:[[@LINE+3]]: type error in `match` pattern type - // CHECK:STDERR: expected: fn () -> choice Ints - // CHECK:STDERR: actual: choice Ints + // CHECK:STDERR: expected: choice Ints + // CHECK:STDERR: actual: fn () -> choice Ints default => { return 1; } } } diff --git a/explorer/testdata/basic_syntax/fail_choice_pattern_extra_parens.carbon b/explorer/testdata/basic_syntax/fail_choice_pattern_extra_parens.carbon new file mode 100644 index 000000000000..998a9df3423e --- /dev/null +++ b/explorer/testdata/basic_syntax/fail_choice_pattern_extra_parens.carbon @@ -0,0 +1,23 @@ +// 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 +// +// AUTOUPDATE +// RUN: %{not} %{explorer-run} +// RUN: %{not} %{explorer-run-trace} + +package ExplorerTest api; + +choice Ints { + None, + One(i32), + Two(i32, i32) +} + +fn Main() -> i32 { + match (Ints.None) { + // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/basic_syntax/fail_choice_pattern_extra_parens.carbon:[[@LINE+1]]: alternative `choice Ints.None` does not expect an argument list + case Ints.None() => { return 0; } + default => { return 1; } + } +} diff --git a/explorer/testdata/basic_syntax/fail_choice_pattern_no_parens.carbon b/explorer/testdata/basic_syntax/fail_choice_pattern_no_parens.carbon new file mode 100644 index 000000000000..ac5d0339595a --- /dev/null +++ b/explorer/testdata/basic_syntax/fail_choice_pattern_no_parens.carbon @@ -0,0 +1,25 @@ +// 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 +// +// AUTOUPDATE +// RUN: %{not} %{explorer-run} +// RUN: %{not} %{explorer-run-trace} + +package ExplorerTest api; + +choice Ints { + None(), + One(i32), + Two(i32, i32) +} + +fn Main() -> i32 { + match (Ints.None()) { + case Ints.None => { return 0; } + // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/basic_syntax/fail_choice_pattern_no_parens.carbon:[[@LINE+3]]: type error in `match` pattern type + // CHECK:STDERR: expected: fn () -> choice Ints + // CHECK:STDERR: actual: choice Ints + default => { return 1; } + } +} diff --git a/explorer/testdata/let/fail_match_choice.carbon b/explorer/testdata/let/fail_match_choice.carbon index 7bb8a61209f9..ec98b8d502a7 100644 --- a/explorer/testdata/let/fail_match_choice.carbon +++ b/explorer/testdata/let/fail_match_choice.carbon @@ -15,13 +15,13 @@ choice Ints { } fn Main() -> i32 { - var x: auto = Ints.None(); + var x: auto = Ints.None; var n: auto = 0; match (x) { case Ints.One(var x: auto) => { x = 2; } - case Ints.None() => { + case Ints.None => { n = n - 1; } case Ints.Two(x: auto, y: auto) => {