Skip to content

Commit 13fbe3c

Browse files
zygoloidzygoloid
authored
Allow interop with classes with virtual base classes. (#6413)
For now, treat such classes as being final, since we can't correctly derive from them. This removes the last category of C++ class that we are entirely unable to interop with, and is a prerequisite for interop with C++ iostreams (which have a virtual base class).
1 parent 8866e39 commit 13fbe3c

File tree

4 files changed

+321
-67
lines changed

4 files changed

+321
-67
lines changed

‎toolchain/check/cpp/import.cpp‎

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -742,6 +742,13 @@ static auto GetInheritanceKind(clang::CXXRecordDecl* class_def)
742742
return SemIR::Class::Final;
743743
}
744744

745+
if (class_def->getNumVBases()) {
746+
// TODO: We treat classes with virtual bases as final for now. We use the
747+
// layout of the class including its virtual bases as its Carbon type
748+
// layout, so we wouldn't behave correctly if we derived from it.
749+
return SemIR::Class::Final;
750+
}
751+
745752
if (class_def->isAbstract()) {
746753
// If the class has any abstract members, it's abstract.
747754
return SemIR::Class::Abstract;
@@ -804,8 +811,15 @@ static auto ImportClassObjectRepr(Context& context, SemIR::ClassId class_id,
804811

805812
// Import bases.
806813
for (const auto& base : clang_def->bases()) {
807-
CARBON_CHECK(!base.isVirtual(),
808-
"Should not import definition for class with a virtual base");
814+
if (base.isVirtual()) {
815+
// If the base is virtual, skip it from the layout. We don't know where it
816+
// will actually appear within the complete object layout, as a pointer to
817+
// this class might point to a derived type that puts the vbase in a
818+
// different place.
819+
// TODO: Track that the virtual base existed. Support derived-to-vbase
820+
// conversions by generating a clang AST fragment.
821+
continue;
822+
}
809823

810824
auto [base_type_inst_id, base_type_id] =
811825
ImportTypeAndDependencies(context, import_ir_inst_id, base.getType());
@@ -844,6 +858,10 @@ static auto ImportClassObjectRepr(Context& context, SemIR::ClassId class_id,
844858
class_info.base_id = SemIR::InstId::None;
845859
}
846860

861+
// TODO: If the base class has virtual bases, the size of the type that we
862+
// add to the layout here will be the full size of the class (including
863+
// virtual bases), whereas the size actually occupied by this base class is
864+
// only the nvsize (excluding virtual bases).
847865
auto base_offset = base.isVirtual()
848866
? clang_layout.getVBaseClassOffset(base_class)
849867
: clang_layout.getBaseClassOffset(base_class);
@@ -2447,17 +2465,6 @@ auto ImportClassDefinitionForClangDecl(Context& context, SemIR::LocId loc_id,
24472465
auto* class_def = class_decl->getDefinition();
24482466
CARBON_CHECK(class_def, "Complete type has no definition");
24492467

2450-
if (class_def->getNumVBases()) {
2451-
// TODO: Handle virtual bases. We don't actually know where they go in the
2452-
// layout. We may also want to use a different size in the layout for
2453-
// `partial C`, excluding the virtual base. It's also not entirely safe to
2454-
// just skip over the virtual base, as the type we would construct would
2455-
// have a misleading size. For now, treat a C++ class with vbases as
2456-
// incomplete in Carbon.
2457-
context.TODO(loc_id, "class with virtual bases");
2458-
return false;
2459-
}
2460-
24612468
BuildClassDefinition(context, import_ir_inst_id, class_id, class_inst_id,
24622469
class_def);
24632470
} else if (auto* enum_decl = dyn_cast<clang::EnumDecl>(clang_decl)) {

‎toolchain/check/testdata/interop/cpp/class/base.carbon‎

Lines changed: 104 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -298,30 +298,128 @@ library "[[@TEST_NAME]]";
298298
import Cpp library "virtual_inheritance.h";
299299

300300
fn Convert(p: Cpp.B*) -> Cpp.A* {
301-
// CHECK:STDERR: fail_todo_use_virtual_inheritance.carbon:[[@LINE+21]]:3: error: semantics TODO: `class with virtual bases` [SemanticsTodo]
301+
// CHECK:STDERR: fail_todo_use_virtual_inheritance.carbon:[[@LINE+7]]:3: error: cannot implicitly convert expression of type `Cpp.B*` to `Cpp.A*` [ConversionFailure]
302302
// CHECK:STDERR: return p;
303303
// CHECK:STDERR: ^~~~~~~~~
304-
// CHECK:STDERR: fail_todo_use_virtual_inheritance.carbon:[[@LINE+18]]:3: note: while completing C++ type `Cpp.B` [InCppTypeCompletion]
304+
// CHECK:STDERR: fail_todo_use_virtual_inheritance.carbon:[[@LINE+4]]:3: note: type `Cpp.B*` does not implement interface `Core.ImplicitAs(Cpp.A*)` [MissingImplInMemberAccessNote]
305305
// CHECK:STDERR: return p;
306306
// CHECK:STDERR: ^~~~~~~~~
307307
// CHECK:STDERR:
308-
// CHECK:STDERR: fail_todo_use_virtual_inheritance.carbon:[[@LINE+14]]:3: error: semantics TODO: `class with virtual bases` [SemanticsTodo]
308+
return p;
309+
}
310+
311+
// --- diamond.h
312+
313+
struct A {
314+
int a;
315+
};
316+
317+
struct B : virtual A {
318+
int b;
319+
};
320+
321+
struct C : virtual A {
322+
int c;
323+
};
324+
325+
struct D : B, C {
326+
int d;
327+
};
328+
329+
// --- fail_todo_use_diamond.carbon
330+
331+
library "[[@TEST_NAME]]";
332+
333+
import Cpp library "diamond.h";
334+
335+
fn ConvertBA(p: Cpp.B*) -> Cpp.A* {
336+
// CHECK:STDERR: fail_todo_use_diamond.carbon:[[@LINE+7]]:3: error: cannot implicitly convert expression of type `Cpp.B*` to `Cpp.A*` [ConversionFailure]
309337
// CHECK:STDERR: return p;
310338
// CHECK:STDERR: ^~~~~~~~~
311-
// CHECK:STDERR: fail_todo_use_virtual_inheritance.carbon:[[@LINE+11]]:3: note: while completing C++ type `Cpp.B` [InCppTypeCompletion]
339+
// CHECK:STDERR: fail_todo_use_diamond.carbon:[[@LINE+4]]:3: note: type `Cpp.B*` does not implement interface `Core.ImplicitAs(Cpp.A*)` [MissingImplInMemberAccessNote]
312340
// CHECK:STDERR: return p;
313341
// CHECK:STDERR: ^~~~~~~~~
314342
// CHECK:STDERR:
315-
// CHECK:STDERR: fail_todo_use_virtual_inheritance.carbon:[[@LINE+7]]:3: error: cannot implicitly convert expression of type `Cpp.B*` to `Cpp.A*` [ConversionFailure]
343+
return p;
344+
}
345+
346+
fn ConvertCA(p: Cpp.C*) -> Cpp.A* {
347+
// CHECK:STDERR: fail_todo_use_diamond.carbon:[[@LINE+7]]:3: error: cannot implicitly convert expression of type `Cpp.C*` to `Cpp.A*` [ConversionFailure]
316348
// CHECK:STDERR: return p;
317349
// CHECK:STDERR: ^~~~~~~~~
318-
// CHECK:STDERR: fail_todo_use_virtual_inheritance.carbon:[[@LINE+4]]:3: note: type `Cpp.B*` does not implement interface `Core.ImplicitAs(Cpp.A*)` [MissingImplInMemberAccessNote]
350+
// CHECK:STDERR: fail_todo_use_diamond.carbon:[[@LINE+4]]:3: note: type `Cpp.C*` does not implement interface `Core.ImplicitAs(Cpp.A*)` [MissingImplInMemberAccessNote]
319351
// CHECK:STDERR: return p;
320352
// CHECK:STDERR: ^~~~~~~~~
321353
// CHECK:STDERR:
322354
return p;
323355
}
324356

357+
fn ConvertDB(p: Cpp.D*) -> Cpp.B* {
358+
// CHECK:STDERR: fail_todo_use_diamond.carbon:[[@LINE+7]]:3: error: cannot implicitly convert expression of type `Cpp.D*` to `Cpp.B*` [ConversionFailure]
359+
// CHECK:STDERR: return p;
360+
// CHECK:STDERR: ^~~~~~~~~
361+
// CHECK:STDERR: fail_todo_use_diamond.carbon:[[@LINE+4]]:3: note: type `Cpp.D*` does not implement interface `Core.ImplicitAs(Cpp.B*)` [MissingImplInMemberAccessNote]
362+
// CHECK:STDERR: return p;
363+
// CHECK:STDERR: ^~~~~~~~~
364+
// CHECK:STDERR:
365+
return p;
366+
}
367+
368+
fn ConvertDC(p: Cpp.D*) -> Cpp.C* {
369+
// CHECK:STDERR: fail_todo_use_diamond.carbon:[[@LINE+7]]:3: error: cannot implicitly convert expression of type `Cpp.D*` to `Cpp.C*` [ConversionFailure]
370+
// CHECK:STDERR: return p;
371+
// CHECK:STDERR: ^~~~~~~~~
372+
// CHECK:STDERR: fail_todo_use_diamond.carbon:[[@LINE+4]]:3: note: type `Cpp.D*` does not implement interface `Core.ImplicitAs(Cpp.C*)` [MissingImplInMemberAccessNote]
373+
// CHECK:STDERR: return p;
374+
// CHECK:STDERR: ^~~~~~~~~
375+
// CHECK:STDERR:
376+
return p;
377+
}
378+
379+
fn AccessBA(d: Cpp.D) -> i32 {
380+
// CHECK:STDERR: fail_todo_use_diamond.carbon:[[@LINE+7]]:11: error: cannot convert expression of type `Cpp.D` to `Cpp.B` with `as` [ConversionFailure]
381+
// CHECK:STDERR: return (d as Cpp.B).b;
382+
// CHECK:STDERR: ^~~~~~~~~~
383+
// CHECK:STDERR: fail_todo_use_diamond.carbon:[[@LINE+4]]:11: note: type `Cpp.D` does not implement interface `Core.As(Cpp.B)` [MissingImplInMemberAccessNote]
384+
// CHECK:STDERR: return (d as Cpp.B).b;
385+
// CHECK:STDERR: ^~~~~~~~~~
386+
// CHECK:STDERR:
387+
return (d as Cpp.B).b;
388+
}
389+
390+
fn AccessCA(d: Cpp.D) -> i32 {
391+
// CHECK:STDERR: fail_todo_use_diamond.carbon:[[@LINE+7]]:11: error: cannot convert expression of type `Cpp.D` to `Cpp.C` with `as` [ConversionFailure]
392+
// CHECK:STDERR: return (d as Cpp.C).b;
393+
// CHECK:STDERR: ^~~~~~~~~~
394+
// CHECK:STDERR: fail_todo_use_diamond.carbon:[[@LINE+4]]:11: note: type `Cpp.D` does not implement interface `Core.As(Cpp.C)` [MissingImplInMemberAccessNote]
395+
// CHECK:STDERR: return (d as Cpp.C).b;
396+
// CHECK:STDERR: ^~~~~~~~~~
397+
// CHECK:STDERR:
398+
return (d as Cpp.C).b;
399+
}
400+
401+
fn AccessDB(d: Cpp.D) -> i32 {
402+
// CHECK:STDERR: fail_todo_use_diamond.carbon:[[@LINE+7]]:10: error: cannot implicitly convert expression of type `Cpp.D` to `Cpp.B` [ConversionFailure]
403+
// CHECK:STDERR: return d.b;
404+
// CHECK:STDERR: ^~~
405+
// CHECK:STDERR: fail_todo_use_diamond.carbon:[[@LINE+4]]:10: note: type `Cpp.D` does not implement interface `Core.ImplicitAs(Cpp.B)` [MissingImplInMemberAccessNote]
406+
// CHECK:STDERR: return d.b;
407+
// CHECK:STDERR: ^~~
408+
// CHECK:STDERR:
409+
return d.b;
410+
}
411+
412+
fn AccessDC(d: Cpp.D) -> i32 {
413+
// CHECK:STDERR: fail_todo_use_diamond.carbon:[[@LINE+7]]:10: error: cannot implicitly convert expression of type `Cpp.D` to `Cpp.C` [ConversionFailure]
414+
// CHECK:STDERR: return d.c;
415+
// CHECK:STDERR: ^~~
416+
// CHECK:STDERR: fail_todo_use_diamond.carbon:[[@LINE+4]]:10: note: type `Cpp.D` does not implement interface `Core.ImplicitAs(Cpp.C)` [MissingImplInMemberAccessNote]
417+
// CHECK:STDERR: return d.c;
418+
// CHECK:STDERR: ^~~
419+
// CHECK:STDERR:
420+
return d.c;
421+
}
422+
325423
// --- final.h
326424

327425
struct A final {};

‎toolchain/check/testdata/interop/cpp/function/operators.carbon‎

Lines changed: 0 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -877,54 +877,6 @@ fn F() {
877877
let result: i32 = *CreateIncomplete() + complete;
878878
}
879879

880-
// ============================================================================
881-
// Unsupported operand type in instantiation
882-
// ============================================================================
883-
884-
// --- unsupported_in_instantiation.h
885-
886-
struct Supported {};
887-
888-
template<typename T>
889-
struct Unsupported : public virtual Supported {};
890-
using UnsupportedAlias = Unsupported<int>;
891-
extern UnsupportedAlias unsupported;
892-
893-
// --- fail_import_unsupported_in_instantiation_unary.carbon
894-
895-
library "[[@TEST_NAME]]";
896-
897-
import Cpp library "unsupported_in_instantiation.h";
898-
899-
fn F() {
900-
// CHECK:STDERR: fail_import_unsupported_in_instantiation_unary.carbon:[[@LINE+7]]:21: error: semantics TODO: `class with virtual bases` [SemanticsTodo]
901-
// CHECK:STDERR: let result: i32 = -Cpp.unsupported;
902-
// CHECK:STDERR: ^~~~~~~~~~~~~~~~
903-
// CHECK:STDERR: fail_import_unsupported_in_instantiation_unary.carbon:[[@LINE+4]]:21: note: while completing C++ type `Cpp.Unsupported` [InCppTypeCompletion]
904-
// CHECK:STDERR: let result: i32 = -Cpp.unsupported;
905-
// CHECK:STDERR: ^~~~~~~~~~~~~~~~
906-
// CHECK:STDERR:
907-
let result: i32 = -Cpp.unsupported;
908-
}
909-
910-
// --- fail_import_unsupported_in_instantiation_binary.carbon
911-
912-
library "[[@TEST_NAME]]";
913-
914-
import Cpp library "unsupported_in_instantiation.h";
915-
916-
fn F() {
917-
var supported: Cpp.Supported = Cpp.Supported.Supported();
918-
// CHECK:STDERR: fail_import_unsupported_in_instantiation_binary.carbon:[[@LINE+7]]:21: error: semantics TODO: `class with virtual bases` [SemanticsTodo]
919-
// CHECK:STDERR: let result: i32 = supported + Cpp.unsupported;
920-
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~
921-
// CHECK:STDERR: fail_import_unsupported_in_instantiation_binary.carbon:[[@LINE+4]]:21: note: while completing C++ type `Cpp.Unsupported` [InCppTypeCompletion]
922-
// CHECK:STDERR: let result: i32 = supported + Cpp.unsupported;
923-
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~
924-
// CHECK:STDERR:
925-
let result: i32 = supported + Cpp.unsupported;
926-
}
927-
928880
// ============================================================================
929881
// Indirect template instantiation
930882
// ============================================================================

0 commit comments

Comments
 (0)