From 878d458c10789a394888cc1cf2df788ae30ef192 Mon Sep 17 00:00:00 2001 From: David Blaikie Date: Wed, 25 Sep 2024 23:20:29 +0000 Subject: [PATCH 1/5] Baseline test --- .../class/fail_adapt_with_base.carbon | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100644 toolchain/check/testdata/class/fail_adapt_with_base.carbon diff --git a/toolchain/check/testdata/class/fail_adapt_with_base.carbon b/toolchain/check/testdata/class/fail_adapt_with_base.carbon new file mode 100644 index 0000000000000..9f9a347151fd2 --- /dev/null +++ b/toolchain/check/testdata/class/fail_adapt_with_base.carbon @@ -0,0 +1,73 @@ +// 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/fail_adapt_with_base.carbon +// TIP: To dump output, run: +// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/class/fail_adapt_with_base.carbon +class Simple {}; +base class AdaptWithVirtual { + adapt Simple; + virtual fn F(); +} + +// CHECK:STDOUT: --- fail_adapt_with_base.carbon +// CHECK:STDOUT: +// CHECK:STDOUT: constants { +// CHECK:STDOUT: %Simple: type = class_type @Simple [template] +// CHECK:STDOUT: %.1: type = struct_type {} [template] +// CHECK:STDOUT: %.2: = complete_type_witness %.1 [template] +// CHECK:STDOUT: %AdaptWithVirtual: type = class_type @AdaptWithVirtual [template] +// CHECK:STDOUT: %.3: type = tuple_type () [template] +// CHECK:STDOUT: %.4: type = ptr_type %.1 [template] +// CHECK:STDOUT: %F.type: type = fn_type @F [template] +// CHECK:STDOUT: %F: %F.type = struct_value () [template] +// CHECK:STDOUT: %.5: = complete_type_witness %Simple [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: .Simple = %Simple.decl +// CHECK:STDOUT: .AdaptWithVirtual = %AdaptWithVirtual.decl +// CHECK:STDOUT: } +// CHECK:STDOUT: %Core.import = import Core +// CHECK:STDOUT: %Simple.decl: type = class_decl @Simple [template = constants.%Simple] {} +// CHECK:STDOUT: %AdaptWithVirtual.decl: type = class_decl @AdaptWithVirtual [template = constants.%AdaptWithVirtual] {} +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: class @Simple { +// CHECK:STDOUT: %.loc6: = complete_type_witness %.1 [template = constants.%.2] +// CHECK:STDOUT: +// CHECK:STDOUT: !members: +// CHECK:STDOUT: .Self = constants.%Simple +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: class @AdaptWithVirtual { +// CHECK:STDOUT: %Simple.ref: type = name_ref Simple, file.%Simple.decl [template = constants.%Simple] +// CHECK:STDOUT: adapt_decl %Simple +// CHECK:STDOUT: %F.decl: %F.type = fn_decl @F [template = constants.%F] {} +// CHECK:STDOUT: %.loc16: = complete_type_witness %Simple [template = constants.%.5] +// CHECK:STDOUT: +// CHECK:STDOUT: !members: +// CHECK:STDOUT: .Self = constants.%AdaptWithVirtual +// CHECK:STDOUT: .F = %F.decl +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: virtual fn @F(); +// CHECK:STDOUT: From a5866578372557d017086b7abacce30c09bc709c Mon Sep 17 00:00:00 2001 From: David Blaikie Date: Thu, 26 Sep 2024 00:07:41 +0000 Subject: [PATCH 2/5] Reject virtual functions in an adapter --- toolchain/check/modifiers.cpp | 9 +++++++-- .../check/testdata/class/fail_adapt_with_base.carbon | 12 +++++++++--- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/toolchain/check/modifiers.cpp b/toolchain/check/modifiers.cpp index e53836fb310ec..5798bdb57373c 100644 --- a/toolchain/check/modifiers.cpp +++ b/toolchain/check/modifiers.cpp @@ -95,13 +95,18 @@ auto CheckMethodModifiersOnFunction( std::optional parent_scope_inst) -> void { if (parent_scope_inst) { if (auto class_decl = parent_scope_inst->TryAs()) { - auto inheritance_kind = - context.classes().Get(class_decl->class_id).inheritance_kind; + auto& class_def = context.classes().Get(class_decl->class_id); + auto inheritance_kind = class_def.inheritance_kind; if (inheritance_kind == SemIR::Class::Final) { ForbidModifiersOnDecl(context, introducer, KeywordModifierSet::Virtual, " in a non-abstract non-base `class` definition", context.insts().GetLocId(parent_scope_inst_id)); } + if (class_def.adapt_id.is_valid()) { + ForbidModifiersOnDecl(context, introducer, KeywordModifierSet::Virtual, + " in an adapter definition", + context.insts().GetLocId(parent_scope_inst_id)); + } if (inheritance_kind != SemIR::Class::Abstract) { ForbidModifiersOnDecl(context, introducer, KeywordModifierSet::Abstract, " in a non-abstract `class` definition", diff --git a/toolchain/check/testdata/class/fail_adapt_with_base.carbon b/toolchain/check/testdata/class/fail_adapt_with_base.carbon index 9f9a347151fd2..9a6874e826437 100644 --- a/toolchain/check/testdata/class/fail_adapt_with_base.carbon +++ b/toolchain/check/testdata/class/fail_adapt_with_base.carbon @@ -10,6 +10,12 @@ class Simple {}; base class AdaptWithVirtual { adapt Simple; + // CHECK:STDERR: fail_adapt_with_base.carbon:[[@LINE+6]]:3: error: `virtual` not allowed on `fn` declaration in an adapter definition + // CHECK:STDERR: virtual fn F(); + // CHECK:STDERR: ^~~~~~~ + // CHECK:STDERR: fail_adapt_with_base.carbon:[[@LINE-5]]:1: note: containing definition here + // CHECK:STDERR: base class AdaptWithVirtual { + // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ virtual fn F(); } @@ -52,7 +58,7 @@ base class AdaptWithVirtual { // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: class @Simple { -// CHECK:STDOUT: %.loc6: = complete_type_witness %.1 [template = constants.%.2] +// CHECK:STDOUT: %.loc10: = complete_type_witness %.1 [template = constants.%.2] // CHECK:STDOUT: // CHECK:STDOUT: !members: // CHECK:STDOUT: .Self = constants.%Simple @@ -62,12 +68,12 @@ base class AdaptWithVirtual { // CHECK:STDOUT: %Simple.ref: type = name_ref Simple, file.%Simple.decl [template = constants.%Simple] // CHECK:STDOUT: adapt_decl %Simple // CHECK:STDOUT: %F.decl: %F.type = fn_decl @F [template = constants.%F] {} -// CHECK:STDOUT: %.loc16: = complete_type_witness %Simple [template = constants.%.5] +// CHECK:STDOUT: %.loc14: = complete_type_witness %Simple [template = constants.%.5] // CHECK:STDOUT: // CHECK:STDOUT: !members: // CHECK:STDOUT: .Self = constants.%AdaptWithVirtual // CHECK:STDOUT: .F = %F.decl // CHECK:STDOUT: } // CHECK:STDOUT: -// CHECK:STDOUT: virtual fn @F(); +// CHECK:STDOUT: fn @F(); // CHECK:STDOUT: From 639b160e3bc9380427bf2f7fef81faba704d5613 Mon Sep 17 00:00:00 2001 From: David Blaikie Date: Thu, 26 Sep 2024 18:00:27 +0000 Subject: [PATCH 3/5] Update test to demonstrate fix incomplete when the adapt comes after the virtual function --- .../testdata/class/fail_adapt_with_base.carbon | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/toolchain/check/testdata/class/fail_adapt_with_base.carbon b/toolchain/check/testdata/class/fail_adapt_with_base.carbon index 9a6874e826437..b7011a1e0e2c5 100644 --- a/toolchain/check/testdata/class/fail_adapt_with_base.carbon +++ b/toolchain/check/testdata/class/fail_adapt_with_base.carbon @@ -9,14 +9,8 @@ // TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/class/fail_adapt_with_base.carbon class Simple {}; base class AdaptWithVirtual { - adapt Simple; - // CHECK:STDERR: fail_adapt_with_base.carbon:[[@LINE+6]]:3: error: `virtual` not allowed on `fn` declaration in an adapter definition - // CHECK:STDERR: virtual fn F(); - // CHECK:STDERR: ^~~~~~~ - // CHECK:STDERR: fail_adapt_with_base.carbon:[[@LINE-5]]:1: note: containing definition here - // CHECK:STDERR: base class AdaptWithVirtual { - // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ virtual fn F(); + adapt Simple; } // CHECK:STDOUT: --- fail_adapt_with_base.carbon @@ -26,10 +20,10 @@ base class AdaptWithVirtual { // CHECK:STDOUT: %.1: type = struct_type {} [template] // CHECK:STDOUT: %.2: = complete_type_witness %.1 [template] // CHECK:STDOUT: %AdaptWithVirtual: type = class_type @AdaptWithVirtual [template] -// CHECK:STDOUT: %.3: type = tuple_type () [template] -// CHECK:STDOUT: %.4: type = ptr_type %.1 [template] // CHECK:STDOUT: %F.type: type = fn_type @F [template] +// CHECK:STDOUT: %.3: type = tuple_type () [template] // CHECK:STDOUT: %F: %F.type = struct_value () [template] +// CHECK:STDOUT: %.4: type = ptr_type %.1 [template] // CHECK:STDOUT: %.5: = complete_type_witness %Simple [template] // CHECK:STDOUT: } // CHECK:STDOUT: @@ -65,15 +59,15 @@ base class AdaptWithVirtual { // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: class @AdaptWithVirtual { +// CHECK:STDOUT: %F.decl: %F.type = fn_decl @F [template = constants.%F] {} // CHECK:STDOUT: %Simple.ref: type = name_ref Simple, file.%Simple.decl [template = constants.%Simple] // CHECK:STDOUT: adapt_decl %Simple -// CHECK:STDOUT: %F.decl: %F.type = fn_decl @F [template = constants.%F] {} -// CHECK:STDOUT: %.loc14: = complete_type_witness %Simple [template = constants.%.5] +// CHECK:STDOUT: %.loc20: = complete_type_witness %Simple [template = constants.%.5] // CHECK:STDOUT: // CHECK:STDOUT: !members: // CHECK:STDOUT: .Self = constants.%AdaptWithVirtual // CHECK:STDOUT: .F = %F.decl // CHECK:STDOUT: } // CHECK:STDOUT: -// CHECK:STDOUT: fn @F(); +// CHECK:STDOUT: virtual fn @F(); // CHECK:STDOUT: From f15e2492857e3c6ae59b9c24f2822d645884450f Mon Sep 17 00:00:00 2001 From: David Blaikie Date: Thu, 26 Sep 2024 23:07:17 +0000 Subject: [PATCH 4/5] Remove the old incomplete fix, implement a end-of-adapter detection instead --- toolchain/check/handle_class.cpp | 19 +++++++++++++++++++ toolchain/check/modifiers.cpp | 9 ++------- .../class/fail_adapt_with_base.carbon | 8 ++++++-- toolchain/diagnostics/diagnostic_kind.def | 2 ++ 4 files changed, 29 insertions(+), 9 deletions(-) diff --git a/toolchain/check/handle_class.cpp b/toolchain/check/handle_class.cpp index 664c04a313aa0..c42fc93fd0acc 100644 --- a/toolchain/check/handle_class.cpp +++ b/toolchain/check/handle_class.cpp @@ -591,6 +591,25 @@ static auto CheckCompleteAdapterClassType(Context& context, return SemIR::InstId::BuiltinError; } + for (auto inst_id : context.inst_block_stack().PeekCurrentBlockContents()) { + if (auto function_decl = + context.insts().TryGetAs(inst_id)) { + auto& function = context.functions().Get(function_decl->function_id); + if (function.virtual_modifier == + SemIR::Function::VirtualModifier::Virtual) { + CARBON_DIAGNOSTIC(AdaptWithVirtual, Error, + "adapter with virtual function"); + CARBON_DIAGNOSTIC(AdaptWithVirtualHere, Note, + "first virtual function declaration is here"); + context.emitter() + .Build(class_info.adapt_id, AdaptWithVirtual) + .Note(inst_id, AdaptWithVirtualHere) + .Emit(); + return SemIR::InstId::BuiltinError; + } + } + } + // The object representation of the adapter is the object representation // of the adapted type. This is the adapted type itself unless it's a class // type. diff --git a/toolchain/check/modifiers.cpp b/toolchain/check/modifiers.cpp index 5798bdb57373c..e53836fb310ec 100644 --- a/toolchain/check/modifiers.cpp +++ b/toolchain/check/modifiers.cpp @@ -95,18 +95,13 @@ auto CheckMethodModifiersOnFunction( std::optional parent_scope_inst) -> void { if (parent_scope_inst) { if (auto class_decl = parent_scope_inst->TryAs()) { - auto& class_def = context.classes().Get(class_decl->class_id); - auto inheritance_kind = class_def.inheritance_kind; + auto inheritance_kind = + context.classes().Get(class_decl->class_id).inheritance_kind; if (inheritance_kind == SemIR::Class::Final) { ForbidModifiersOnDecl(context, introducer, KeywordModifierSet::Virtual, " in a non-abstract non-base `class` definition", context.insts().GetLocId(parent_scope_inst_id)); } - if (class_def.adapt_id.is_valid()) { - ForbidModifiersOnDecl(context, introducer, KeywordModifierSet::Virtual, - " in an adapter definition", - context.insts().GetLocId(parent_scope_inst_id)); - } if (inheritance_kind != SemIR::Class::Abstract) { ForbidModifiersOnDecl(context, introducer, KeywordModifierSet::Abstract, " in a non-abstract `class` definition", diff --git a/toolchain/check/testdata/class/fail_adapt_with_base.carbon b/toolchain/check/testdata/class/fail_adapt_with_base.carbon index b7011a1e0e2c5..b95170a22ed5d 100644 --- a/toolchain/check/testdata/class/fail_adapt_with_base.carbon +++ b/toolchain/check/testdata/class/fail_adapt_with_base.carbon @@ -10,6 +10,12 @@ class Simple {}; base class AdaptWithVirtual { virtual fn F(); + // CHECK:STDERR: fail_adapt_with_base.carbon:[[@LINE+6]]:3: error: adapter with virtual function + // CHECK:STDERR: adapt Simple; + // CHECK:STDERR: ^~~~~~~~~~~~~ + // CHECK:STDERR: fail_adapt_with_base.carbon:[[@LINE-4]]:3: note: first virtual function declaration is here + // CHECK:STDERR: virtual fn F(); + // CHECK:STDERR: ^~~~~~~~~~~~~~~ adapt Simple; } @@ -24,7 +30,6 @@ base class AdaptWithVirtual { // CHECK:STDOUT: %.3: type = tuple_type () [template] // CHECK:STDOUT: %F: %F.type = struct_value () [template] // CHECK:STDOUT: %.4: type = ptr_type %.1 [template] -// CHECK:STDOUT: %.5: = complete_type_witness %Simple [template] // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: imports { @@ -62,7 +67,6 @@ base class AdaptWithVirtual { // CHECK:STDOUT: %F.decl: %F.type = fn_decl @F [template = constants.%F] {} // CHECK:STDOUT: %Simple.ref: type = name_ref Simple, file.%Simple.decl [template = constants.%Simple] // CHECK:STDOUT: adapt_decl %Simple -// CHECK:STDOUT: %.loc20: = complete_type_witness %Simple [template = constants.%.5] // CHECK:STDOUT: // CHECK:STDOUT: !members: // CHECK:STDOUT: .Self = constants.%AdaptWithVirtual diff --git a/toolchain/diagnostics/diagnostic_kind.def b/toolchain/diagnostics/diagnostic_kind.def index e01f3386d00df..8168b93d64523 100644 --- a/toolchain/diagnostics/diagnostic_kind.def +++ b/toolchain/diagnostics/diagnostic_kind.def @@ -205,8 +205,10 @@ CARBON_DIAGNOSTIC_KIND(InvalidBuiltinSignature) CARBON_DIAGNOSTIC_KIND(AdaptDeclRepeated) CARBON_DIAGNOSTIC_KIND(AdaptWithBase) CARBON_DIAGNOSTIC_KIND(AdaptWithFields) +CARBON_DIAGNOSTIC_KIND(AdaptWithVirtual) CARBON_DIAGNOSTIC_KIND(AdaptWithBaseHere) CARBON_DIAGNOSTIC_KIND(AdaptWithFieldHere) +CARBON_DIAGNOSTIC_KIND(AdaptWithVirtualHere) CARBON_DIAGNOSTIC_KIND(BaseDeclRepeated) CARBON_DIAGNOSTIC_KIND(BaseIsFinal) CARBON_DIAGNOSTIC_KIND(BaseMissingExtend) From d5f985a58ccc2f6b77d7bc6f77a0d552b60e9841 Mon Sep 17 00:00:00 2001 From: David Blaikie Date: Wed, 2 Oct 2024 17:48:46 +0000 Subject: [PATCH 5/5] Update tests --- toolchain/check/testdata/class/fail_adapt_with_base.carbon | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/toolchain/check/testdata/class/fail_adapt_with_base.carbon b/toolchain/check/testdata/class/fail_adapt_with_base.carbon index b95170a22ed5d..6e5e3b0e43bdc 100644 --- a/toolchain/check/testdata/class/fail_adapt_with_base.carbon +++ b/toolchain/check/testdata/class/fail_adapt_with_base.carbon @@ -52,8 +52,8 @@ base class AdaptWithVirtual { // CHECK:STDOUT: .AdaptWithVirtual = %AdaptWithVirtual.decl // CHECK:STDOUT: } // CHECK:STDOUT: %Core.import = import Core -// CHECK:STDOUT: %Simple.decl: type = class_decl @Simple [template = constants.%Simple] {} -// CHECK:STDOUT: %AdaptWithVirtual.decl: type = class_decl @AdaptWithVirtual [template = constants.%AdaptWithVirtual] {} +// CHECK:STDOUT: %Simple.decl: type = class_decl @Simple [template = constants.%Simple] {} {} +// CHECK:STDOUT: %AdaptWithVirtual.decl: type = class_decl @AdaptWithVirtual [template = constants.%AdaptWithVirtual] {} {} // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: class @Simple { @@ -64,7 +64,7 @@ base class AdaptWithVirtual { // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: class @AdaptWithVirtual { -// CHECK:STDOUT: %F.decl: %F.type = fn_decl @F [template = constants.%F] {} +// CHECK:STDOUT: %F.decl: %F.type = fn_decl @F [template = constants.%F] {} {} // CHECK:STDOUT: %Simple.ref: type = name_ref Simple, file.%Simple.decl [template = constants.%Simple] // CHECK:STDOUT: adapt_decl %Simple // CHECK:STDOUT: