From 5806d8385d796b35edd96653e23c9b05ef732cce Mon Sep 17 00:00:00 2001 From: David Blaikie Date: Tue, 10 Sep 2024 11:16:59 -0700 Subject: [PATCH] Add SemIR support for virtual functions (#4272) I guess this technically would also allow code to pass check that hasn't before, and that isn't covered by tests (since it's masked by other failures in the tests that already test this functionality) - should I add another test/add some code to a valid test case? Also, this'll miscompile in lowering, since there's no support there yet - should I do anything about that to make lowering fail in some way? Or is it acceptable that some things just silently mis-lower? (I could add a currently-miscompiling test case too, to demonstrate this? (not sure if the autogenerated tests leave space for comments that would explain that the currently-tested behavior is incorrect?)) Is the addition to EntityWithParamsBase suitable? of course not all functions can be virtual, so it's a wasted bit at the moment for all those cases (though it's free, since it's bitpacked - but as we want to add more bits in there it might not be a scalable solution)? --------- Co-authored-by: Jon Ross-Perkins --- toolchain/check/handle_function.cpp | 17 ++- ...rs.carbon => todo_access_modifiers.carbon} | 90 +--------------- .../testdata/class/virtual_modifiers.carbon | 100 ++++++++++++++++++ toolchain/sem_ir/formatter.cpp | 20 +++- toolchain/sem_ir/function.h | 7 ++ 5 files changed, 142 insertions(+), 92 deletions(-) rename toolchain/check/testdata/class/{fail_todo_modifiers.carbon => todo_access_modifiers.carbon} (53%) create mode 100644 toolchain/check/testdata/class/virtual_modifiers.carbon diff --git a/toolchain/check/handle_function.cpp b/toolchain/check/handle_function.cpp index 34d655ee4ee7..0064b9edf684 100644 --- a/toolchain/check/handle_function.cpp +++ b/toolchain/check/handle_function.cpp @@ -194,9 +194,17 @@ static auto BuildFunctionDecl(Context& context, DiagnoseModifiers(context, introducer, is_definition, parent_scope_inst_id, parent_scope_inst); bool is_extern = introducer.modifier_set.HasAnyOf(KeywordModifierSet::Extern); - if (introducer.modifier_set.HasAnyOf(KeywordModifierSet::Method)) { - context.TODO(introducer.modifier_node_id(ModifierOrder::Decl), - "method modifier"); + SemIR::FunctionFields::VirtualModifier virtual_modifier = + SemIR::FunctionFields::VirtualModifier::None; + + if (introducer.modifier_set.HasAnyOf(KeywordModifierSet::Virtual)) { + virtual_modifier = SemIR::FunctionFields::VirtualModifier::Virtual; + } + if (introducer.modifier_set.HasAnyOf(KeywordModifierSet::Abstract)) { + virtual_modifier = SemIR::FunctionFields::VirtualModifier::Abstract; + } + if (introducer.modifier_set.HasAnyOf(KeywordModifierSet::Impl)) { + virtual_modifier = SemIR::FunctionFields::VirtualModifier::Impl; } if (introducer.modifier_set.HasAnyOf(KeywordModifierSet::Interface)) { // TODO: Once we are saving the modifiers for a function, add check that @@ -216,7 +224,8 @@ static auto BuildFunctionDecl(Context& context, auto function_info = SemIR::Function{{name_context.MakeEntityWithParamsBase( name, decl_id, is_extern, introducer.extern_library)}, - {.return_storage_id = return_storage_id}}; + {.return_storage_id = return_storage_id, + .virtual_modifier = virtual_modifier}}; if (is_definition) { function_info.definition_id = decl_id; } diff --git a/toolchain/check/testdata/class/fail_todo_modifiers.carbon b/toolchain/check/testdata/class/todo_access_modifiers.carbon similarity index 53% rename from toolchain/check/testdata/class/fail_todo_modifiers.carbon rename to toolchain/check/testdata/class/todo_access_modifiers.carbon index 0e3f01e8839a..3a9ca04efee4 100644 --- a/toolchain/check/testdata/class/fail_todo_modifiers.carbon +++ b/toolchain/check/testdata/class/todo_access_modifiers.carbon @@ -4,9 +4,9 @@ // // AUTOUPDATE // TIP: To test this file alone, run: -// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/class/fail_todo_modifiers.carbon +// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/class/todo_access_modifiers.carbon // TIP: To dump output, run: -// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/class/fail_todo_modifiers.carbon +// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/class/todo_access_modifiers.carbon // TODO: Test calls to these (member access control is not yet implemented). class Access { @@ -19,42 +19,7 @@ class Access { protected var l: i32; } -base class Base { - - // CHECK:STDERR: fail_todo_modifiers.carbon:[[@LINE+4]]:3: ERROR: Semantics TODO: `method modifier`. - // CHECK:STDERR: virtual fn H(); - // CHECK:STDERR: ^~~~~~~ - // CHECK:STDERR: - virtual fn H(); - - // CHECK:STDERR: fail_todo_modifiers.carbon:[[@LINE+4]]:3: ERROR: Semantics TODO: `method modifier`. - // CHECK:STDERR: impl fn I(); - // CHECK:STDERR: ^~~~ - // CHECK:STDERR: - impl fn I(); -} - -abstract class Abstract { - - // CHECK:STDERR: fail_todo_modifiers.carbon:[[@LINE+4]]:3: ERROR: Semantics TODO: `method modifier`. - // CHECK:STDERR: abstract fn J(); - // CHECK:STDERR: ^~~~~~~~ - // CHECK:STDERR: - abstract fn J(); - - // CHECK:STDERR: fail_todo_modifiers.carbon:[[@LINE+4]]:3: ERROR: Semantics TODO: `method modifier`. - // CHECK:STDERR: virtual fn K(); - // CHECK:STDERR: ^~~~~~~ - // CHECK:STDERR: - virtual fn K(); - - // CHECK:STDERR: fail_todo_modifiers.carbon:[[@LINE+3]]:3: ERROR: Semantics TODO: `method modifier`. - // CHECK:STDERR: impl fn L(); - // CHECK:STDERR: ^~~~ - impl fn L(); -} - -// CHECK:STDOUT: --- fail_todo_modifiers.carbon +// CHECK:STDOUT: --- todo_access_modifiers.carbon // CHECK:STDOUT: // CHECK:STDOUT: constants { // CHECK:STDOUT: %Access: type = class_type @Access [template] @@ -67,19 +32,6 @@ abstract class Abstract { // CHECK:STDOUT: %Int32: %Int32.type = struct_value () [template] // CHECK:STDOUT: %.2: type = unbound_element_type %Access, i32 [template] // CHECK:STDOUT: %.3: type = struct_type {.k: i32, .l: i32} [template] -// CHECK:STDOUT: %Base: type = class_type @Base [template] -// CHECK:STDOUT: %H.type: type = fn_type @H [template] -// CHECK:STDOUT: %H: %H.type = struct_value () [template] -// CHECK:STDOUT: %I.type: type = fn_type @I [template] -// CHECK:STDOUT: %I: %I.type = struct_value () [template] -// CHECK:STDOUT: %.4: type = struct_type {} [template] -// CHECK:STDOUT: %Abstract: type = class_type @Abstract [template] -// CHECK:STDOUT: %J.type: type = fn_type @J [template] -// CHECK:STDOUT: %J: %J.type = struct_value () [template] -// CHECK:STDOUT: %K.type: type = fn_type @K [template] -// CHECK:STDOUT: %K: %K.type = struct_value () [template] -// CHECK:STDOUT: %L.type: type = fn_type @L [template] -// CHECK:STDOUT: %L: %L.type = struct_value () [template] // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: imports { @@ -101,13 +53,9 @@ abstract class Abstract { // CHECK:STDOUT: package: = namespace [template] { // CHECK:STDOUT: .Core = imports.%Core // CHECK:STDOUT: .Access = %Access.decl -// CHECK:STDOUT: .Base = %Base.decl -// CHECK:STDOUT: .Abstract = %Abstract.decl // CHECK:STDOUT: } // CHECK:STDOUT: %Core.import = import Core // CHECK:STDOUT: %Access.decl: type = class_decl @Access [template = constants.%Access] {} -// CHECK:STDOUT: %Base.decl: type = class_decl @Base [template = constants.%Base] {} -// CHECK:STDOUT: %Abstract.decl: type = class_decl @Abstract [template = constants.%Abstract] {} // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: class @Access { @@ -130,41 +78,9 @@ abstract class Abstract { // CHECK:STDOUT: .l [protected] = %.loc19_18 // CHECK:STDOUT: } // CHECK:STDOUT: -// CHECK:STDOUT: class @Base { -// CHECK:STDOUT: %H.decl: %H.type = fn_decl @H [template = constants.%H] {} -// CHECK:STDOUT: %I.decl: %I.type = fn_decl @I [template = constants.%I] {} -// CHECK:STDOUT: -// CHECK:STDOUT: !members: -// CHECK:STDOUT: .Self = constants.%Base -// CHECK:STDOUT: .H = %H.decl -// CHECK:STDOUT: .I = %I.decl -// CHECK:STDOUT: } -// CHECK:STDOUT: -// CHECK:STDOUT: class @Abstract { -// CHECK:STDOUT: %J.decl: %J.type = fn_decl @J [template = constants.%J] {} -// CHECK:STDOUT: %K.decl: %K.type = fn_decl @K [template = constants.%K] {} -// CHECK:STDOUT: %L.decl: %L.type = fn_decl @L [template = constants.%L] {} -// CHECK:STDOUT: -// CHECK:STDOUT: !members: -// CHECK:STDOUT: .Self = constants.%Abstract -// CHECK:STDOUT: .J = %J.decl -// CHECK:STDOUT: .K = %K.decl -// CHECK:STDOUT: .L = %L.decl -// CHECK:STDOUT: } -// CHECK:STDOUT: // CHECK:STDOUT: fn @F(); // CHECK:STDOUT: // CHECK:STDOUT: fn @G(); // CHECK:STDOUT: // CHECK:STDOUT: fn @Int32() -> type = "int.make_type_32"; // CHECK:STDOUT: -// CHECK:STDOUT: fn @H(); -// CHECK:STDOUT: -// CHECK:STDOUT: fn @I(); -// CHECK:STDOUT: -// CHECK:STDOUT: fn @J(); -// CHECK:STDOUT: -// CHECK:STDOUT: fn @K(); -// CHECK:STDOUT: -// CHECK:STDOUT: fn @L(); -// CHECK:STDOUT: diff --git a/toolchain/check/testdata/class/virtual_modifiers.carbon b/toolchain/check/testdata/class/virtual_modifiers.carbon new file mode 100644 index 000000000000..efab3db1bb1e --- /dev/null +++ b/toolchain/check/testdata/class/virtual_modifiers.carbon @@ -0,0 +1,100 @@ +// 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 +// TIP: To test this file alone, run: +// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/class/virtual_modifiers.carbon +// TIP: To dump output, run: +// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/class/virtual_modifiers.carbon + + +base class Base { + virtual fn H(); + + impl fn I(); +} + +abstract class Abstract { + abstract fn J(); + + virtual fn K(); + + impl fn L(); +} + +// CHECK:STDOUT: --- virtual_modifiers.carbon +// CHECK:STDOUT: +// CHECK:STDOUT: constants { +// CHECK:STDOUT: %Base: type = class_type @Base [template] +// CHECK:STDOUT: %H.type: type = fn_type @H [template] +// CHECK:STDOUT: %.1: type = tuple_type () [template] +// CHECK:STDOUT: %H: %H.type = struct_value () [template] +// CHECK:STDOUT: %I.type: type = fn_type @I [template] +// CHECK:STDOUT: %I: %I.type = struct_value () [template] +// CHECK:STDOUT: %.2: type = struct_type {} [template] +// CHECK:STDOUT: %Abstract: type = class_type @Abstract [template] +// CHECK:STDOUT: %J.type: type = fn_type @J [template] +// CHECK:STDOUT: %J: %J.type = struct_value () [template] +// CHECK:STDOUT: %K.type: type = fn_type @K [template] +// CHECK:STDOUT: %K: %K.type = struct_value () [template] +// CHECK:STDOUT: %L.type: type = fn_type @L [template] +// CHECK:STDOUT: %L: %L.type = struct_value () [template] +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: imports { +// CHECK:STDOUT: %Core: = namespace file.%Core.import, [template] { +// CHECK:STDOUT: import Core//prelude +// CHECK:STDOUT: import Core//prelude/operators +// CHECK:STDOUT: import Core//prelude/types +// CHECK:STDOUT: import Core//prelude/operators/arithmetic +// CHECK:STDOUT: import Core//prelude/operators/as +// CHECK:STDOUT: import Core//prelude/operators/bitwise +// CHECK:STDOUT: import Core//prelude/operators/comparison +// CHECK:STDOUT: import Core//prelude/types/bool +// CHECK:STDOUT: } +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: file { +// CHECK:STDOUT: package: = namespace [template] { +// CHECK:STDOUT: .Core = imports.%Core +// CHECK:STDOUT: .Base = %Base.decl +// CHECK:STDOUT: .Abstract = %Abstract.decl +// CHECK:STDOUT: } +// CHECK:STDOUT: %Core.import = import Core +// CHECK:STDOUT: %Base.decl: type = class_decl @Base [template = constants.%Base] {} +// CHECK:STDOUT: %Abstract.decl: type = class_decl @Abstract [template = constants.%Abstract] {} +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: class @Base { +// CHECK:STDOUT: %H.decl: %H.type = fn_decl @H [template = constants.%H] {} +// CHECK:STDOUT: %I.decl: %I.type = fn_decl @I [template = constants.%I] {} +// CHECK:STDOUT: +// CHECK:STDOUT: !members: +// CHECK:STDOUT: .Self = constants.%Base +// CHECK:STDOUT: .H = %H.decl +// CHECK:STDOUT: .I = %I.decl +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: class @Abstract { +// CHECK:STDOUT: %J.decl: %J.type = fn_decl @J [template = constants.%J] {} +// CHECK:STDOUT: %K.decl: %K.type = fn_decl @K [template = constants.%K] {} +// CHECK:STDOUT: %L.decl: %L.type = fn_decl @L [template = constants.%L] {} +// CHECK:STDOUT: +// CHECK:STDOUT: !members: +// CHECK:STDOUT: .Self = constants.%Abstract +// CHECK:STDOUT: .J = %J.decl +// CHECK:STDOUT: .K = %K.decl +// CHECK:STDOUT: .L = %L.decl +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: virtual fn @H(); +// CHECK:STDOUT: +// CHECK:STDOUT: impl fn @I(); +// CHECK:STDOUT: +// CHECK:STDOUT: abstract fn @J(); +// CHECK:STDOUT: +// CHECK:STDOUT: virtual fn @K(); +// CHECK:STDOUT: +// CHECK:STDOUT: impl fn @L(); +// CHECK:STDOUT: diff --git a/toolchain/sem_ir/formatter.cpp b/toolchain/sem_ir/formatter.cpp index 2e145dab1f43..c1a742ead08f 100644 --- a/toolchain/sem_ir/formatter.cpp +++ b/toolchain/sem_ir/formatter.cpp @@ -261,7 +261,25 @@ class FormatterImpl { // Formats a full function. auto FormatFunction(FunctionId id) -> void { const Function& fn = sem_ir_.functions().Get(id); - FormatEntityStart(fn.is_extern ? "extern fn" : "fn", fn.generic_id, id); + std::string function_start; + switch (fn.virtual_modifier) { + case FunctionFields::VirtualModifier::Virtual: + function_start += "virtual "; + break; + case FunctionFields::VirtualModifier::Abstract: + function_start += "abstract "; + break; + case FunctionFields::VirtualModifier::Impl: + function_start += "impl "; + break; + case FunctionFields::VirtualModifier::None: + break; + } + if (fn.is_extern) { + function_start += "extern "; + } + function_start += "fn"; + FormatEntityStart(function_start, fn.generic_id, id); llvm::SaveAndRestore function_scope(scope_, inst_namer_->GetScopeFor(id)); diff --git a/toolchain/sem_ir/function.h b/toolchain/sem_ir/function.h index 43aaed965419..4fd512a32363 100644 --- a/toolchain/sem_ir/function.h +++ b/toolchain/sem_ir/function.h @@ -14,6 +14,9 @@ namespace Carbon::SemIR { // Function-specific fields. struct FunctionFields { + // Kinds of virtual modifiers that can apply to functions. + enum class VirtualModifier { None, Virtual, Abstract, Impl }; + // The following members always have values, and do not change throughout the // lifetime of the function. @@ -23,6 +26,10 @@ struct FunctionFields { // always present if the function has a declared return type. InstId return_storage_id; + // Which, if any, virtual modifier (virtual, abstract, or impl) is applied to + // this function. + VirtualModifier virtual_modifier; + // The following member is set on the first call to the function, or at the // point where the function is defined.