From 798a40c886d47331375435522d1f8dba453feeec Mon Sep 17 00:00:00 2001 From: Adrien Leravat Date: Wed, 4 Jan 2023 11:09:23 -0800 Subject: [PATCH] Basic support for `impl` virtual override keyword (#2493) Features: * Add basic support for `impl` virtual methods (override virtual method) * Error on invalid declaration for `impl` and `virtual`, covering simple use cases * Add `abstract` fn parser-only support Changes: * Modify parser to handle new function specifiers, resolve conflicts * Group `virtual_override` and `FN`, and group `impl_kind` and `IMPL` to avoid ambiguities with around `impl` token parsing * Add new `VirtualOverride` enum for function declarations, and matching `virt_override() -> VirtualOverride` getter * Update function declaration logic Depends on #2462 --- explorer/ast/ast_test_matchers_test.cpp | 6 +-- explorer/ast/declaration.cpp | 5 ++- explorer/ast/declaration.h | 24 ++++++----- explorer/interpreter/type_checker.cpp | 40 +++++++++++++++++-- explorer/interpreter/value.cpp | 2 +- explorer/syntax/parser.ypp | 22 +++++----- .../fail_abstract_method_not_supported.carbon | 24 +++++++++++ .../fail_impl_method_not_existing.carbon | 24 +++++++++++ .../class/fail_impl_method_not_virtual.carbon | 27 +++++++++++++ ...l_override_virtual_with_non_virtual.carbon | 22 ++++++++++ .../fail_redeclare_virtual_method.carbon | 23 +++++++++++ explorer/testdata/class/virtual_method.carbon | 3 +- .../testdata/class/virtual_method_self.carbon | 6 +-- .../class/virtual_method_shadowed_attr.carbon | 3 +- .../testdata/impl/fail_bad_member_kind.carbon | 2 +- .../testdata/mixin/fail_mix_in_impl.carbon | 2 +- 16 files changed, 197 insertions(+), 38 deletions(-) create mode 100644 explorer/testdata/class/fail_abstract_method_not_supported.carbon create mode 100644 explorer/testdata/class/fail_impl_method_not_existing.carbon create mode 100644 explorer/testdata/class/fail_impl_method_not_virtual.carbon create mode 100644 explorer/testdata/class/fail_override_virtual_with_non_virtual.carbon create mode 100644 explorer/testdata/class/fail_redeclare_virtual_method.carbon diff --git a/explorer/ast/ast_test_matchers_test.cpp b/explorer/ast/ast_test_matchers_test.cpp index d6ccfddf6603..9c9474607c53 100644 --- a/explorer/ast/ast_test_matchers_test.cpp +++ b/explorer/ast/ast_test_matchers_test.cpp @@ -103,7 +103,7 @@ TEST(MatchesFunctionDeclarationTest, BasicUsage) { Block body(DummyLoc, {}); FunctionDeclaration decl(DummyLoc, "Foo", {}, std::nullopt, ¶ms, ReturnTerm::Omitted(DummyLoc), &body, - /*is_virtual=*/false); + VirtualOverride::None); EXPECT_THAT(decl, MatchesFunctionDeclaration()); EXPECT_THAT(&decl, MatchesFunctionDeclaration()); @@ -117,7 +117,7 @@ TEST(MatchesFunctionDeclarationTest, BasicUsage) { FunctionDeclaration forward_decl(DummyLoc, "Foo", {}, std::nullopt, ¶ms, ReturnTerm::Omitted(DummyLoc), std::nullopt, - /*is_virtual=*/false); + VirtualOverride::None); EXPECT_THAT(forward_decl, MatchesFunctionDeclaration().WithName("Foo")); EXPECT_THAT(forward_decl, Not(MatchesFunctionDeclaration().WithBody(_))); @@ -151,7 +151,7 @@ TEST(ASTDeclarationsTest, BasicUsage) { Block body(DummyLoc, {}); FunctionDeclaration decl(DummyLoc, "Foo", {}, std::nullopt, ¶ms, ReturnTerm::Omitted(DummyLoc), &body, - /*is_virtual=*/false); + VirtualOverride::None); AST ast = {.declarations = {&decl}}; EXPECT_THAT(ast, ASTDeclarations(ElementsAre(MatchesFunctionDeclaration()))); diff --git a/explorer/ast/declaration.cpp b/explorer/ast/declaration.cpp index ae6092425155..f9a9eb1a08ca 100644 --- a/explorer/ast/declaration.cpp +++ b/explorer/ast/declaration.cpp @@ -342,14 +342,15 @@ auto FunctionDeclaration::Create(Nonnull arena, Nonnull param_pattern, ReturnTerm return_term, std::optional> body, - bool is_virtual) + VirtualOverride virt_override) -> ErrorOr> { DeducedParameters split_params; CARBON_ASSIGN_OR_RETURN(split_params, SplitDeducedParameters(source_loc, deduced_params)); return arena->New( source_loc, name, std::move(split_params.resolved_params), - split_params.self_pattern, param_pattern, return_term, body, is_virtual); + split_params.self_pattern, param_pattern, return_term, body, + virt_override); } void CallableDeclaration::PrintDepth(int depth, llvm::raw_ostream& out) const { diff --git a/explorer/ast/declaration.h b/explorer/ast/declaration.h index 4694fa7ccad5..310daf39e5b8 100644 --- a/explorer/ast/declaration.h +++ b/explorer/ast/declaration.h @@ -125,6 +125,9 @@ class Declaration : public AstNode { bool is_type_checked_ = false; }; +// A function's virtual override keyword. +enum class VirtualOverride { None, Abstract, Virtual, Impl }; + class CallableDeclaration : public Declaration { public: CallableDeclaration(AstNodeKind kind, SourceLocation loc, std::string name, @@ -132,7 +135,8 @@ class CallableDeclaration : public Declaration { std::optional> self_pattern, Nonnull param_pattern, ReturnTerm return_term, - std::optional> body, bool is_virtual) + std::optional> body, + VirtualOverride virt_override) : Declaration(kind, loc), name_(std::move(name)), deduced_parameters_(std::move(deduced_params)), @@ -140,7 +144,7 @@ class CallableDeclaration : public Declaration { param_pattern_(param_pattern), return_term_(return_term), body_(body), - is_virtual_(is_virtual) {} + virt_override_(virt_override) {} void PrintDepth(int depth, llvm::raw_ostream& out) const; @@ -161,7 +165,7 @@ class CallableDeclaration : public Declaration { auto return_term() -> ReturnTerm& { return return_term_; } auto body() const -> std::optional> { return body_; } auto body() -> std::optional> { return body_; } - auto is_virtual() const -> bool { return is_virtual_; } + auto virt_override() const -> VirtualOverride { return virt_override_; } auto value_category() const -> ValueCategory { return ValueCategory::Let; } @@ -174,7 +178,7 @@ class CallableDeclaration : public Declaration { Nonnull param_pattern_; ReturnTerm return_term_; std::optional> body_; - bool is_virtual_; + VirtualOverride virt_override_; }; class FunctionDeclaration : public CallableDeclaration { @@ -186,7 +190,8 @@ class FunctionDeclaration : public CallableDeclaration { std::vector> deduced_params, Nonnull param_pattern, ReturnTerm return_term, - std::optional> body, bool is_virtual) + std::optional> body, + VirtualOverride virt_override) -> ErrorOr>; // Use `Create()` instead. This is public only so Arena::New() can call it. @@ -195,11 +200,12 @@ class FunctionDeclaration : public CallableDeclaration { std::optional> self_pattern, Nonnull param_pattern, ReturnTerm return_term, - std::optional> body, bool is_virtual) + std::optional> body, + VirtualOverride virt_override) : CallableDeclaration(AstNodeKind::FunctionDeclaration, source_loc, std::move(name), std::move(deduced_params), self_pattern, param_pattern, return_term, body, - is_virtual) {} + virt_override) {} static auto classof(const AstNode* node) -> bool { return InheritsFromFunctionDeclaration(node->kind()); @@ -227,8 +233,8 @@ class DestructorDeclaration : public CallableDeclaration { : CallableDeclaration(AstNodeKind::DestructorDeclaration, source_loc, "destructor", std::move(deduced_params), self_pattern, param_pattern, return_term, body, - // TODO: Add virtual destructors. - /*is_virtual=*/false) {} + // TODO: Add virtual destructors + VirtualOverride::None) {} static auto classof(const AstNode* node) -> bool { return InheritsFromDestructorDeclaration(node->kind()); diff --git a/explorer/interpreter/type_checker.cpp b/explorer/interpreter/type_checker.cpp index ba2db216211c..118a94294cc2 100644 --- a/explorer/interpreter/type_checker.cpp +++ b/explorer/interpreter/type_checker.cpp @@ -4511,16 +4511,48 @@ auto TypeChecker::DeclareClassDeclaration(Nonnull class_decl, const int class_level = base_class ? (*base_class)->hierarchy_level() + 1 : 0; for (const auto* m : class_decl->members()) { const auto* fun = dyn_cast(m); - if (!fun || !fun->is_virtual()) { + if (!fun) { continue; } - // TODO: Implement complete declaration logic from - // /docs/design/classes.md#virtual-methods. - if (!fun->is_method()) { + if (fun->virt_override() != VirtualOverride::None && !fun->is_method()) { return ProgramError(fun->source_loc()) << "Error declaring `" << fun->name() << "`" << ": class functions cannot be virtual."; } + bool has_vtable_entry = + class_vtable.find(fun->name()) != class_vtable.end(); + // TODO: Implement complete declaration logic from + // `/docs/design/classes.md#virtual-methods`. + switch (fun->virt_override()) { + case VirtualOverride::Abstract: + // Not supported yet. + return ProgramError(fun->source_loc()) + << "Error declaring `" << fun->name() << "`" + << ": `abstract` methods are not yet supported."; + case VirtualOverride::None: + case VirtualOverride::Virtual: + if (has_vtable_entry) { + return ProgramError(fun->source_loc()) + << "Error declaring `" << fun->name() << "`" + << ": method is declared virtual in base class, use `impl` " + "to override it."; + } + // TODO: Error if declaring virtual method shadowing non-virtual method. + // See https://github.com/carbon-language/carbon-lang/issues/2355. + if (fun->virt_override() == VirtualOverride::None) { + // Not added to the vtable. + continue; + } + break; + case VirtualOverride::Impl: + if (!has_vtable_entry) { + return ProgramError(fun->source_loc()) + << "Error declaring `" << fun->name() << "`" + << ": cannot override a method that is not declared " + "`abstract` or `virtual` in base class."; + } + break; + } class_vtable[fun->name()] = {fun, class_level}; } diff --git a/explorer/interpreter/value.cpp b/explorer/interpreter/value.cpp index ef981ed596ab..16c8486b74d7 100644 --- a/explorer/interpreter/value.cpp +++ b/explorer/interpreter/value.cpp @@ -161,7 +161,7 @@ static auto GetNamedElement(Nonnull arena, Nonnull v, } else if ((*func)->declaration().is_method()) { // Found a method. Turn it into a bound method. const auto& m = cast(**func); - if (!m.declaration().is_virtual()) { + if (m.declaration().virt_override() == VirtualOverride::None) { return arena->New(&m.declaration(), me_value, &class_type.bindings()); } diff --git a/explorer/syntax/parser.ypp b/explorer/syntax/parser.ypp index 2df82c08694e..d8adf0611e4d 100644 --- a/explorer/syntax/parser.ypp +++ b/explorer/syntax/parser.ypp @@ -97,14 +97,14 @@ %token sized_type_literal %token string_literal %type designator -%type impl_kind +%type impl_kind_intro %type > impl_type %type > package_directive %type import_directive %type > import_directives %type optional_library_path %type api_or_impl -%type fn_virtual_override_intro +%type fn_virtual_override_intro %type class_declaration_extensibility %type >> class_declaration_extends %type > declaration @@ -1067,9 +1067,13 @@ impl_deduced_params: // This includes the FN keyword to work around a shift-reduce conflict between virtual function's `IMPL FN` and interfaces `IMPL`. fn_virtual_override_intro: FN - { $$ = false; } + { $$ = VirtualOverride::None; } +| ABSTRACT FN + { $$ = VirtualOverride::Abstract; } | VIRTUAL FN - { $$ = true; } + { $$ = VirtualOverride::Virtual; } +| IMPL FN + { $$ = VirtualOverride::Impl; } ; function_declaration: fn_virtual_override_intro identifier deduced_params maybe_empty_tuple_pattern return_term block @@ -1210,10 +1214,10 @@ declaration: $$ = arena->New(arena, context.source_loc(), $2, $3, $5); } -| impl_kind IMPL impl_deduced_params impl_type AS type_or_where_expression LEFT_CURLY_BRACE impl_body RIGHT_CURLY_BRACE +| impl_kind_intro impl_deduced_params impl_type AS type_or_where_expression LEFT_CURLY_BRACE impl_body RIGHT_CURLY_BRACE { ErrorOr impl = ImplDeclaration::Create( - arena, context.source_loc(), $1, $4, $6, $3, $8); + arena, context.source_loc(), $1, $3, $5, $2, $7); if (impl.ok()) { $$ = *impl; } else { @@ -1224,10 +1228,10 @@ declaration: | alias_declaration { $$ = $1; } ; -impl_kind: - // Internal +impl_kind_intro: + IMPL // Internal { $$ = Carbon::ImplKind::InternalImpl; } -| EXTERNAL +| EXTERNAL IMPL { $$ = Carbon::ImplKind::ExternalImpl; } ; impl_type: diff --git a/explorer/testdata/class/fail_abstract_method_not_supported.carbon b/explorer/testdata/class/fail_abstract_method_not_supported.carbon new file mode 100644 index 000000000000..9ba1d62fe908 --- /dev/null +++ b/explorer/testdata/class/fail_abstract_method_not_supported.carbon @@ -0,0 +1,24 @@ +// 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; + +base class C { +} + +class D extends C { + abstract fn Foo[self:Self]() -> i32 { + return 1; + // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/class/fail_abstract_method_not_supported.carbon:[[@LINE+1]]: Error declaring `Foo`: `abstract` methods are not yet supported. + } +} + +fn Main() -> i32 { + let d: D = {}; + return 0; +} diff --git a/explorer/testdata/class/fail_impl_method_not_existing.carbon b/explorer/testdata/class/fail_impl_method_not_existing.carbon new file mode 100644 index 000000000000..305abe8c6dbe --- /dev/null +++ b/explorer/testdata/class/fail_impl_method_not_existing.carbon @@ -0,0 +1,24 @@ +// 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; + +base class C { +} + +class D extends C { + impl fn Foo[self:Self]() -> i32 { + return 1; + // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/class/fail_impl_method_not_existing.carbon:[[@LINE+1]]: Error declaring `Foo`: cannot override a method that is not declared `abstract` or `virtual` in base class. + } +} + +fn Main() -> i32 { + let d: D = {}; + return 0; +} diff --git a/explorer/testdata/class/fail_impl_method_not_virtual.carbon b/explorer/testdata/class/fail_impl_method_not_virtual.carbon new file mode 100644 index 000000000000..48380261f3ee --- /dev/null +++ b/explorer/testdata/class/fail_impl_method_not_virtual.carbon @@ -0,0 +1,27 @@ +// 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; + +base class C { + fn Foo[self:Self]() -> i32 { + return 1; + } +} + +class D extends C { + impl fn Foo[self:Self]() -> i32 { + return 1; + // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/class/fail_impl_method_not_virtual.carbon:[[@LINE+1]]: Error declaring `Foo`: cannot override a method that is not declared `abstract` or `virtual` in base class. + } +} + +fn Main() -> i32 { + let c: C = {}; + return 0; +} diff --git a/explorer/testdata/class/fail_override_virtual_with_non_virtual.carbon b/explorer/testdata/class/fail_override_virtual_with_non_virtual.carbon new file mode 100644 index 000000000000..a4ce89ddbc2c --- /dev/null +++ b/explorer/testdata/class/fail_override_virtual_with_non_virtual.carbon @@ -0,0 +1,22 @@ +// 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; + +base class A { + virtual fn Foo[self: Self]() {} +} + +class B extends A { + // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/class/fail_override_virtual_with_non_virtual.carbon:[[@LINE+1]]: Error declaring `Foo`: method is declared virtual in base class, use `impl` to override it. + fn Foo[self: Self]() {} +} + +fn Main() -> i32 { + return 0; +} diff --git a/explorer/testdata/class/fail_redeclare_virtual_method.carbon b/explorer/testdata/class/fail_redeclare_virtual_method.carbon new file mode 100644 index 000000000000..e2b52d5ec358 --- /dev/null +++ b/explorer/testdata/class/fail_redeclare_virtual_method.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; + +base class C { + virtual fn Foo[self:Self]() {} +} + +class D extends C { + // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/class/fail_redeclare_virtual_method.carbon:[[@LINE+1]]: Error declaring `Foo`: method is declared virtual in base class, use `impl` to override it. + virtual fn Foo[self:Self]() {} +} + +fn Main() -> i32 { + let c: C = {}; + return 0; +} diff --git a/explorer/testdata/class/virtual_method.carbon b/explorer/testdata/class/virtual_method.carbon index 69575af8f4cb..31a80ae80d03 100644 --- a/explorer/testdata/class/virtual_method.carbon +++ b/explorer/testdata/class/virtual_method.carbon @@ -25,8 +25,7 @@ base class C { } class D extends C { - // TODO: This should instead use `impl` when supported - virtual fn Foo[self: Self]() -> i32 { + impl fn Foo[self: Self]() -> i32 { return 3; } fn Bar[self: Self]() -> i32 { diff --git a/explorer/testdata/class/virtual_method_self.carbon b/explorer/testdata/class/virtual_method_self.carbon index f4285bc52f4b..4abd0360de32 100644 --- a/explorer/testdata/class/virtual_method_self.carbon +++ b/explorer/testdata/class/virtual_method_self.carbon @@ -23,16 +23,14 @@ base class C { base class D extends C { var value_d: i32; - // TODO: This should instead use `impl` when supported - virtual fn Foo[self: Self]() -> i32 { + impl fn Foo[self: Self]() -> i32 { return self.value_d; } } class E extends D { var value_e: i32; - // TODO: This should instead use `impl` when supported - virtual fn Foo[self: Self]() -> i32 { + impl fn Foo[self: Self]() -> i32 { return self.value_e; } } diff --git a/explorer/testdata/class/virtual_method_shadowed_attr.carbon b/explorer/testdata/class/virtual_method_shadowed_attr.carbon index 5fe047d99e58..e4ea4052382e 100644 --- a/explorer/testdata/class/virtual_method_shadowed_attr.carbon +++ b/explorer/testdata/class/virtual_method_shadowed_attr.carbon @@ -21,8 +21,7 @@ base class C { base class D extends C { var value: i32; - // TODO: This should instead use `impl` when supported - virtual fn Foo[self: Self]() -> i32 { + impl fn Foo[self: Self]() -> i32 { return self.value; } } diff --git a/explorer/testdata/impl/fail_bad_member_kind.carbon b/explorer/testdata/impl/fail_bad_member_kind.carbon index d18200f76a15..03a240bc50db 100644 --- a/explorer/testdata/impl/fail_bad_member_kind.carbon +++ b/explorer/testdata/impl/fail_bad_member_kind.carbon @@ -14,7 +14,7 @@ interface A { } external impl i32 as A { - // CHECK:STDERR: SYNTAX ERROR: {{.*}}/explorer/testdata/impl/fail_bad_member_kind.carbon:[[@LINE+1]]: syntax error, unexpected CLASS, expecting ALIAS or FN or RIGHT_CURLY_BRACE or VIRTUAL + // CHECK:STDERR: SYNTAX ERROR: {{.*}}/explorer/testdata/impl/fail_bad_member_kind.carbon:[[@LINE+1]]: syntax error, unexpected CLASS class T {} } diff --git a/explorer/testdata/mixin/fail_mix_in_impl.carbon b/explorer/testdata/mixin/fail_mix_in_impl.carbon index 0473ca1322da..cb5cc5119a4e 100644 --- a/explorer/testdata/mixin/fail_mix_in_impl.carbon +++ b/explorer/testdata/mixin/fail_mix_in_impl.carbon @@ -19,7 +19,7 @@ interface A { } external impl i32 as A { - // CHECK:STDERR: SYNTAX ERROR: {{.*}}/explorer/testdata/mixin/fail_mix_in_impl.carbon:[[@LINE+1]]: syntax error, unexpected MIX, expecting ALIAS or FN or RIGHT_CURLY_BRACE or VIRTUAL + // CHECK:STDERR: SYNTAX ERROR: {{.*}}/explorer/testdata/mixin/fail_mix_in_impl.carbon:[[@LINE+1]]: syntax error, unexpected MIX __mix Operations; fn F() {} }