From b79d9adeca88547c401ee4fa971e49baf247e829 Mon Sep 17 00:00:00 2001 From: Dana Jansens Date: Wed, 8 Apr 2026 16:09:03 -0400 Subject: [PATCH] Avoid crashing in custom witness for FacetTypes and symbolic object representations (#7033) The type must be complete to look for a witness for Destroy. Do this check through type completion rather than just checking to see if the ClassInfo says the definition is closed, since completing the type has side effects (resolves the self specific definition). Then look for whether the class is abstract through the CompleteTypeInfo instead of just looking at the inheritance type on ClassInfo, like type completion does. Last, FacetTypes are trivially destroyed just like TypeType. --- toolchain/check/custom_witness.cpp | 57 +++++++------ .../impl/custom_witness/destroy.carbon | 79 +++++++++++++++++++ toolchain/check/type_completion.cpp | 4 +- toolchain/sem_ir/type_info.h | 8 ++ 4 files changed, 122 insertions(+), 26 deletions(-) create mode 100644 toolchain/check/testdata/impl/custom_witness/destroy.carbon diff --git a/toolchain/check/custom_witness.cpp b/toolchain/check/custom_witness.cpp index 520cb3c530b2..b43d9b3a851b 100644 --- a/toolchain/check/custom_witness.cpp +++ b/toolchain/check/custom_witness.cpp @@ -18,6 +18,7 @@ #include "toolchain/sem_ir/associated_constant.h" #include "toolchain/sem_ir/builtin_function_kind.h" #include "toolchain/sem_ir/ids.h" +#include "toolchain/sem_ir/type_info.h" #include "toolchain/sem_ir/typed_insts.h" namespace Carbon::Check { @@ -133,16 +134,16 @@ static auto HasWitnessForOneField( // Returns true if `class_type` should impl `Destroy`. static auto CanDestroyClass( Context& context, SemIR::LocId loc_id, SemIR::ClassType class_type, + const SemIR::CompleteTypeInfo& complete_info, SemIR::SpecificInterfaceId query_specific_interface_id, bool is_partial) -> DestroyFormat { - auto class_info = context.classes().Get(class_type.class_id); - // Incomplete and abstract classes can't be destroyed. - if (!class_info.is_complete() || - (!is_partial && class_info.inheritance_kind == - SemIR::Class::InheritanceKind::Abstract)) { + // Abstract classes can't be destroyed. + if (!is_partial && complete_info.IsAbstract()) { return DestroyFormat::NoDestroy; } + auto class_info = context.classes().Get(class_type.class_id); + // `LookupCppImpl` handles C++ types. if (context.name_scopes().Get(class_info.scope_id).is_cpp_scope()) { return DestroyFormat::NoDestroy; @@ -166,23 +167,33 @@ static auto CanDestroyType( context.specific_interfaces().Get(query_specific_interface_id); auto destroy_interface_id = query_specific_interface.interface_id; - auto inst = context.insts().Get(context.constant_values().GetInstId( - GetCanonicalFacetOrTypeValue(context, query_self_const_id))); + auto inst_id = context.constant_values().GetInstId( + GetCanonicalFacetOrTypeValue(context, query_self_const_id)); + auto inst = context.insts().Get(inst_id); + + // For facet values, look if the FacetType provides the same. + if (auto facet_type = + context.types().TryGetAs(inst.type_id())) { + const auto& info = context.facet_types().Get(facet_type->facet_type_id); + for (auto interface : info.extend_constraints) { + if (interface.interface_id == destroy_interface_id) { + return DestroyFormat::Trivial; + } + } + return DestroyFormat::NoDestroy; + } + + // Incomplete types can not be destroyed. + auto type_id = context.types().GetTypeIdForTypeInstId(inst_id); + if (!TryToCompleteType(context, type_id, loc_id)) { + return DestroyFormat::NoDestroy; + } CARBON_KIND_SWITCH(inst) { case SemIR::ImplWitnessAccess::Kind: case SemIR::SymbolicBinding::Kind: { - // These are symbolic, so should never reach `MakeDestroyOpBody`. - // For facet values, look if the FacetType provides the same. - if (auto facet_type = - context.types().TryGetAs(inst.type_id())) { - const auto& info = context.facet_types().Get(facet_type->facet_type_id); - for (auto interface : info.extend_constraints) { - if (interface.interface_id == destroy_interface_id) { - return DestroyFormat::Trivial; - } - } - } + // A symbolic facet of type `type`. Such symbolic values can't be + // destroyed. return DestroyFormat::NoDestroy; } @@ -207,6 +218,7 @@ static auto CanDestroyType( case CARBON_KIND(SemIR::ClassType class_type): { return CanDestroyClass(context, loc_id, class_type, + context.types().GetCompleteTypeInfo(type_id), query_specific_interface_id, /*is_partial=*/false); } @@ -228,6 +240,7 @@ static auto CanDestroyType( auto class_type = context.insts().GetAs(partial_type.inner_id); return CanDestroyClass(context, loc_id, class_type, + context.types().GetCompleteTypeInfo(type_id), query_specific_interface_id, /*is_partial=*/true); } @@ -251,12 +264,6 @@ static auto CanDestroyType( return has_witness ? DestroyFormat::NonTrivial : DestroyFormat::NoDestroy; } - case CARBON_KIND(SemIR::SymbolicBindingType sym_binding): { - return HasWitnessForOneField(context, loc_id, - sym_binding.facet_value_inst_id, - query_specific_interface_id); - } - case CARBON_KIND(SemIR::TupleType tuple_type): { auto block = context.inst_blocks().Get(tuple_type.type_elements_id); if (block.empty()) { @@ -277,7 +284,9 @@ static auto CanDestroyType( } case SemIR::BoolType::Kind: + case SemIR::FacetType::Kind: case SemIR::FloatType::Kind: + case SemIR::IntLiteralType::Kind: case SemIR::IntType::Kind: case SemIR::PointerType::Kind: case SemIR::TypeType::Kind: diff --git a/toolchain/check/testdata/impl/custom_witness/destroy.carbon b/toolchain/check/testdata/impl/custom_witness/destroy.carbon new file mode 100644 index 000000000000..4d13db0dd0b8 --- /dev/null +++ b/toolchain/check/testdata/impl/custom_witness/destroy.carbon @@ -0,0 +1,79 @@ +// 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/int.carbon +// +// AUTOUPDATE +// TIP: To test this file alone, run: +// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/impl/custom_witness/destroy.carbon +// TIP: To dump output, run: +// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/impl/custom_witness/destroy.carbon + +// --- destroy_type.carbon +library "[[@TEST_NAME]]"; + +fn F() { + // This looks for a Destroy witness for a TypeType. + type as Core.Destroy; +} + +// --- destroy_facet_type.carbon +library "[[@TEST_NAME]]"; + +interface Z {} + +fn F() { + // This looks for a Destroy witness for a FacetType. + (Z & Core.Destroy) as Core.Destroy; +} + +// --- destroy_int_literal_type.carbon +library "[[@TEST_NAME]]"; + +fn F() { + // This looks for a Destroy witness for an IntLiteralType. + Core.IntLiteral() as Core.Destroy; +} + +// --- destroy_int_type.carbon +library "[[@TEST_NAME]]"; + +fn F() { + // This looks for a Destroy witness for a class type that adapts IntType, and + // has a symbolic CompleteTypeWitness. + Core.Int(32) as Core.Destroy; +} + +// --- destroy_symbolic_int_type.carbon +library "[[@TEST_NAME]]"; + +fn F(N:! Core.IntLiteral()) { + // This looks for a Destroy witness for a class type that adapts IntType with a + // symbolic type. + Core.Int(N) as Core.Destroy; +} + +// --- destroy_class_type.carbon +library "[[@TEST_NAME]]"; + +class C(T:! type) {} + +fn F() { + // This looks for a Destroy witness for a class type. + C({}) as Core.Destroy; +} + +// --- fail_destroy_incomplete_class.carbon +library "[[@TEST_NAME]]"; + +class C(T:! type); + +fn F() { + // This looks for a Destroy witness for an incomplete class type. + // CHECK:STDERR: fail_destroy_incomplete_class.carbon:[[@LINE+4]]:3: error: cannot convert type `C({})` into type implementing `Core.Destroy` [ConversionFailureTypeToFacet] + // CHECK:STDERR: C({}) as Core.Destroy; + // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~ + // CHECK:STDERR: + C({}) as Core.Destroy; +} diff --git a/toolchain/check/type_completion.cpp b/toolchain/check/type_completion.cpp index d8ebc1e31482..cc7d79b29e65 100644 --- a/toolchain/check/type_completion.cpp +++ b/toolchain/check/type_completion.cpp @@ -820,11 +820,11 @@ auto RequireConcreteType(Context& context, SemIR::TypeId type_id, // TODO: For symbolic types, should add an implicit constraint that they are // not abstract. - auto complete_info = context.types().GetCompleteTypeInfo(type_id); + const auto& complete_info = context.types().GetCompleteTypeInfo(type_id); CARBON_CHECK(complete_info.value_repr.type_id.has_value(), "RequireConcreteType called for an incomplete type. Call " "RequireCompleteType first."); - if (!complete_info.abstract_class_id.has_value()) { + if (!complete_info.IsAbstract()) { return true; } diff --git a/toolchain/sem_ir/type_info.h b/toolchain/sem_ir/type_info.h index 45b4686edca4..9a899633337e 100644 --- a/toolchain/sem_ir/type_info.h +++ b/toolchain/sem_ir/type_info.h @@ -83,6 +83,14 @@ struct CompleteTypeInfo : public Printable { // If this type is abstract, this is id of an abstract class it uses. ClassId abstract_class_id = ClassId::None; + + // Returns whether the type is abstract. + // + // The type must be completed before we can determine if it's abstract. + auto IsAbstract() const -> bool { + CARBON_CHECK(value_repr.kind != ValueRepr::Unknown); + return abstract_class_id.has_value(); + } }; // The representation to use for an initializing expression of some type.