diff --git a/toolchain/check/class.cpp b/toolchain/check/class.cpp index f8064174502f..f71a0a97b528 100644 --- a/toolchain/check/class.cpp +++ b/toolchain/check/class.cpp @@ -18,6 +18,7 @@ #include "toolchain/check/pattern_match.h" #include "toolchain/check/thunk.h" #include "toolchain/check/type.h" +#include "toolchain/diagnostics/format_providers.h" #include "toolchain/parse/node_ids.h" #include "toolchain/sem_ir/builtin_function_kind.h" #include "toolchain/sem_ir/function.h" @@ -138,6 +139,41 @@ static auto AddStructTypeFields( return fields_id; } +// Result of comparing a virtual function in a base class with a potential +// overrider in a derived class. +enum class OverrideMatchResult : uint8_t { + // The functions match. + Match, + // The potential overrider is not marked `override`. + NotAnOverride, + // The names do not match. + NameMismatch, + // The arity (number of explicit parameters) does not match. + ArityMismatch, +}; + +// Compares a virtual function in a base class with a potential overrider in a +// derived class. +static auto CompareVirtualWithOverrider(const SemIR::Function& base_fn, + const SemIR::Function& derived_fn) + -> OverrideMatchResult { + if (derived_fn.virtual_modifier != + SemIR::FunctionFields::VirtualModifier::Override) { + return OverrideMatchResult::NotAnOverride; + } + if (derived_fn.name_id != base_fn.name_id) { + return OverrideMatchResult::NameMismatch; + } + if (derived_fn.call_param_ranges.explicit_size() != + base_fn.call_param_ranges.explicit_size()) { + return OverrideMatchResult::ArityMismatch; + } + // TODO: We should check more thoroughly for compatibility between the two + // functions here, so that we can determine which function is being overridden + // if the base function is in a C++ overload set. + return OverrideMatchResult::Match; +} + // Builds and returns a vtable for the current class. Assumes that the virtual // functions for the class are listed as the top element of the `vtable_stack`. static auto BuildVtable(Context& context, Parse::ClassDefinitionId node_id, @@ -188,6 +224,9 @@ static auto BuildVtable(Context& context, Parse::ClassDefinitionId node_id, llvm::SmallVector vtable; Set implemented_impls; bool carbon_native_vtable = true; + + // Add vtable entries from the base class, updating them to point to a derived + // class overrider if there is one. if (base_vtable_id.has_value()) { const auto& base_vtable = context.vtables().Get(base_vtable_id); carbon_native_vtable = base_vtable.carbon_native_vtable; @@ -216,9 +255,8 @@ static auto BuildVtable(Context& context, Parse::ClassDefinitionId node_id, context.insts() .GetAs(override_fn_decl_id) .function_id); - return override_fn.virtual_modifier == - SemIR::FunctionFields::VirtualModifier::Override && - override_fn.name_id == fn.name_id; + return CompareVirtualWithOverrider(fn, override_fn) == + OverrideMatchResult::Match; }); if (i != vtable_contents.end()) { auto override_fn_id = @@ -264,6 +302,8 @@ static auto BuildVtable(Context& context, Parse::ClassDefinitionId node_id, } } + // Add any remaining virtual functions from the derived class to the vtable, + // and diagnose any `override fn`s that didn't override anything. for (auto inst_id : vtable_contents) { auto fn_decl = context.insts().GetAs(inst_id); auto& fn = context.functions().Get(fn_decl.function_id); @@ -274,8 +314,47 @@ static auto BuildVtable(Context& context, Parse::ClassDefinitionId node_id, } else if (!implemented_impls.Lookup(fn_decl.function_id)) { CARBON_DIAGNOSTIC(OverrideWithoutVirtualInBase, Error, "override without compatible virtual in base class"); - context.emitter().Emit(SemIR::LocId(inst_id), - OverrideWithoutVirtualInBase); + CARBON_DIAGNOSTIC(OverrideCandidateArityMismatch, Note, + "base class function has {2:more|fewer} parameters " + "({0} vs {1} excluding `self`)", + Diagnostics::IntAsSelect, Diagnostics::IntAsSelect, + Diagnostics::BoolAsSelect); + auto builder = context.emitter().Build(SemIR::LocId(inst_id), + OverrideWithoutVirtualInBase); + if (base_vtable_id.has_value()) { + const auto& base_vtable = context.vtables().Get(base_vtable_id); + auto base_vtable_inst_block = + context.inst_blocks().Get(base_vtable.virtual_functions_id); + for (auto base_vtable_entry_id : base_vtable_inst_block) { + if (!base_vtable_entry_id.has_value()) { + continue; + } + auto [derived_vtable_entry_id, derived_vtable_entry_const_id, fn_id, + specific_id] = + DecomposeVirtualFunction(context.sem_ir(), base_vtable_entry_id, + base_class_specific_id); + const auto& base_fn = context.sem_ir().functions().Get(fn_id); + switch (CompareVirtualWithOverrider(base_fn, fn)) { + case OverrideMatchResult::ArityMismatch: + builder.Note(base_fn.first_owning_decl_id, + OverrideCandidateArityMismatch, + base_fn.call_param_ranges.explicit_size() - 1, + fn.call_param_ranges.explicit_size() - 1, + base_fn.call_param_ranges.explicit_size() > + fn.call_param_ranges.explicit_size()); + break; + case OverrideMatchResult::NameMismatch: + // TODO: If the name is similar enough and the overrider otherwise + // matches, emit a note about the potential misspelling. + break; + case OverrideMatchResult::Match: + CARBON_FATAL("Unexpectedly found a matching overrider"); + case OverrideMatchResult::NotAnOverride: + CARBON_FATAL("Should only consider `override fn`s here"); + } + } + } + builder.Emit(); } } diff --git a/toolchain/check/testdata/class/method/virtual.carbon b/toolchain/check/testdata/class/method/virtual.carbon index 0a4ab3683088..6016f28ac5e0 100644 --- a/toolchain/check/testdata/class/method/virtual.carbon +++ b/toolchain/check/testdata/class/method/virtual.carbon @@ -202,13 +202,10 @@ base class Base { class Derived { extend base: Base; - // CHECK:STDERR: fail_impl_mismatch.carbon:[[@LINE+10]]:3: error: 0 arguments passed to function expecting 1 argument [CallArgCountMismatch] + // CHECK:STDERR: fail_impl_mismatch.carbon:[[@LINE+7]]:3: error: override without compatible virtual in base class [OverrideWithoutVirtualInBase] // CHECK:STDERR: override fn F(self, v: i32); // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ - // CHECK:STDERR: fail_impl_mismatch.carbon:[[@LINE+7]]:3: note: calling function declared here [InCallToEntity] - // CHECK:STDERR: override fn F(self, v: i32); - // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ - // CHECK:STDERR: fail_impl_mismatch.carbon:[[@LINE-11]]:3: note: while building thunk to match the signature of this function [ThunkSignature] + // CHECK:STDERR: fail_impl_mismatch.carbon:[[@LINE-8]]:3: note: base class function has fewer parameters (0 vs 1 excluding `self`) [OverrideCandidateArityMismatch] // CHECK:STDERR: virtual fn F(self); // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~ // CHECK:STDERR: diff --git a/toolchain/check/testdata/interop/cpp/class/import/override.carbon b/toolchain/check/testdata/interop/cpp/class/import/override.carbon new file mode 100644 index 000000000000..b45723b85482 --- /dev/null +++ b/toolchain/check/testdata/interop/cpp/class/import/override.carbon @@ -0,0 +1,122 @@ +// 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 +// +// INCLUDE-FILE: toolchain/testing/testdata/min_prelude/convert.carbon +// +// AUTOUPDATE +// TIP: To test this file alone, run: +// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/interop/cpp/class/import/override.carbon +// TIP: To dump output, run: +// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/interop/cpp/class/import/override.carbon + +// --- overload.h + +struct X {}; +struct Y {}; + +struct OverloadedBase { + virtual void f(X x); + virtual void f(Y y); + virtual void f(X x, Y y); +}; + +// --- overload_arity.carbon + +library "[[@TEST_NAME]]"; + +import Cpp library "overload.h"; + +class Overload2Params { + extend base: Cpp.OverloadedBase; + override fn f(self, x: Cpp.X, y: Cpp.Y); +} + +// --- fail_overload_no_matching_arity.carbon + +library "[[@TEST_NAME]]"; + +import Cpp library "overload.h"; + +class Overload0Params { + extend base: Cpp.OverloadedBase; + // + // CHECK:STDERR: fail_overload_no_matching_arity.carbon:[[@LINE+16]]:3: error: override without compatible virtual in base class [OverrideWithoutVirtualInBase] + // CHECK:STDERR: override fn f(self); + // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~ + // CHECK:STDERR: fail_overload_no_matching_arity.carbon:[[@LINE-8]]:10: in file included here [InCppInclude] + // CHECK:STDERR: ./overload.h:6:16: note: base class function has more parameters (1 vs 0 excluding `self`) [OverrideCandidateArityMismatch] + // CHECK:STDERR: virtual void f(X x); + // CHECK:STDERR: ^ + // CHECK:STDERR: fail_overload_no_matching_arity.carbon:[[@LINE-12]]:10: in file included here [InCppInclude] + // CHECK:STDERR: ./overload.h:7:16: note: base class function has more parameters (1 vs 0 excluding `self`) [OverrideCandidateArityMismatch] + // CHECK:STDERR: virtual void f(Y y); + // CHECK:STDERR: ^ + // CHECK:STDERR: fail_overload_no_matching_arity.carbon:[[@LINE-16]]:10: in file included here [InCppInclude] + // CHECK:STDERR: ./overload.h:8:16: note: base class function has more parameters (2 vs 0 excluding `self`) [OverrideCandidateArityMismatch] + // CHECK:STDERR: virtual void f(X x, Y y); + // CHECK:STDERR: ^ + // CHECK:STDERR: + override fn f(self); +} + +class Overload3Params { + extend base: Cpp.OverloadedBase; + // + // CHECK:STDERR: fail_overload_no_matching_arity.carbon:[[@LINE+16]]:3: error: override without compatible virtual in base class [OverrideWithoutVirtualInBase] + // CHECK:STDERR: override fn f(self, x: Cpp.X, y: Cpp.Y, z: Cpp.X); + // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + // CHECK:STDERR: fail_overload_no_matching_arity.carbon:[[@LINE-30]]:10: in file included here [InCppInclude] + // CHECK:STDERR: ./overload.h:6:16: note: base class function has fewer parameters (1 vs 3 excluding `self`) [OverrideCandidateArityMismatch] + // CHECK:STDERR: virtual void f(X x); + // CHECK:STDERR: ^ + // CHECK:STDERR: fail_overload_no_matching_arity.carbon:[[@LINE-34]]:10: in file included here [InCppInclude] + // CHECK:STDERR: ./overload.h:7:16: note: base class function has fewer parameters (1 vs 3 excluding `self`) [OverrideCandidateArityMismatch] + // CHECK:STDERR: virtual void f(Y y); + // CHECK:STDERR: ^ + // CHECK:STDERR: fail_overload_no_matching_arity.carbon:[[@LINE-38]]:10: in file included here [InCppInclude] + // CHECK:STDERR: ./overload.h:8:16: note: base class function has fewer parameters (2 vs 3 excluding `self`) [OverrideCandidateArityMismatch] + // CHECK:STDERR: virtual void f(X x, Y y); + // CHECK:STDERR: ^ + // CHECK:STDERR: + override fn f(self, x: Cpp.X, y: Cpp.Y, z: Cpp.X); +} + +// --- fail_overload_type.carbon +// CHECK:STDERR: fail_overload_type.carbon: error: cannot implicitly convert expression of type `Cpp.Y` to `Cpp.X` [ConversionFailure] +// CHECK:STDERR: fail_overload_type.carbon: note: type `Cpp.Y` does not implement interface `Core.ImplicitAs(Cpp.X)` [MissingImplInMemberAccessInContext] +// TODO: The conversion diagnostics here should have a location. + +library "[[@TEST_NAME]]"; + +import Cpp library "overload.h"; + +class OverloadX { + extend base: Cpp.OverloadedBase; + // + // CHECK:STDERR: fail_overload_type.carbon:[[@LINE+10]]:23: note: initializing function parameter [InCallToFunctionParam] + // CHECK:STDERR: override fn f(self, x: Cpp.X); + // CHECK:STDERR: ^~~~~~~~ + // CHECK:STDERR: fail_overload_type.carbon:[[@LINE-8]]:10: in file included here [InCppInclude] + // CHECK:STDERR: ./overload.h:7:16: note: while building thunk to match the signature of this function [ThunkSignature] + // CHECK:STDERR: virtual void f(Y y); + // CHECK:STDERR: ^ + // CHECK:STDERR: + // CHECK:STDERR: fail_overload_type.carbon: error: cannot implicitly convert expression of type `Cpp.X` to `Cpp.Y` [ConversionFailure] + // CHECK:STDERR: fail_overload_type.carbon: note: type `Cpp.X` does not implement interface `Core.ImplicitAs(Cpp.Y)` [MissingImplInMemberAccessInContext] + override fn f(self, x: Cpp.X); +} + +class OverloadY { + extend base: Cpp.OverloadedBase; + // + // CHECK:STDERR: fail_overload_type.carbon:[[@LINE+8]]:23: note: initializing function parameter [InCallToFunctionParam] + // CHECK:STDERR: override fn f(self, y: Cpp.Y); + // CHECK:STDERR: ^~~~~~~~ + // CHECK:STDERR: fail_overload_type.carbon:[[@LINE-24]]:10: in file included here [InCppInclude] + // CHECK:STDERR: ./overload.h:6:16: note: while building thunk to match the signature of this function [ThunkSignature] + // CHECK:STDERR: virtual void f(X x); + // CHECK:STDERR: ^ + // CHECK:STDERR: + override fn f(self, y: Cpp.Y); +} diff --git a/toolchain/diagnostics/kind.def b/toolchain/diagnostics/kind.def index cc334e19109c..02921d2667a0 100644 --- a/toolchain/diagnostics/kind.def +++ b/toolchain/diagnostics/kind.def @@ -318,6 +318,7 @@ CARBON_DIAGNOSTIC_KIND(FieldWithTuplePattern) CARBON_DIAGNOSTIC_KIND(GenericVirtual) CARBON_DIAGNOSTIC_KIND(OverrideWithoutBase) CARBON_DIAGNOSTIC_KIND(OverrideWithoutVirtualInBase) +CARBON_DIAGNOSTIC_KIND(OverrideCandidateArityMismatch) CARBON_DIAGNOSTIC_KIND(VirtualWithoutSelf) // Deduction.