From acb7810e323394dd8266ad11f2d2067a48683040 Mon Sep 17 00:00:00 2001 From: Dana Jansens Date: Thu, 13 Nov 2025 12:54:49 -0500 Subject: [PATCH] Avoid crashing when an impl decl has a missing definition (#6349) When the missing definition is diagnosed at the end of the file, the witness is set to an error. Impl lookup was skipping impls entirely when the witness was an error, which means a non-final LookupImplWitness could be later evaluated against a specific and crash since the lookup fails instead of returning the error. The same crash could also occur when verifying poisoned queries hadn't changed, but now it can find an ErrorInst witness instead, so it is changed to handle that gracefully. --- toolchain/check/check_unit.cpp | 5 + toolchain/check/impl_lookup.cpp | 13 +- .../check/testdata/impl/error_recovery.carbon | 249 +++++++++++++++--- toolchain/sem_ir/type_iterator.h | 3 +- 4 files changed, 227 insertions(+), 43 deletions(-) diff --git a/toolchain/check/check_unit.cpp b/toolchain/check/check_unit.cpp index e1aca867a99b..5a9b4c110a97 100644 --- a/toolchain/check/check_unit.cpp +++ b/toolchain/check/check_unit.cpp @@ -529,6 +529,11 @@ auto CheckUnit::CheckPoisonedConcreteImplLookupQueries() -> void { /*poison_final_results=*/false); CARBON_CHECK(witness_result.has_final_value()); auto found_witness_id = witness_result.final_witness(); + if (found_witness_id == SemIR::ErrorInst::InstId) { + // Errors may have been diagnosed with the impl used in the poisoned query + // in the meantime (such as a missing definition). + continue; + } if (found_witness_id != poison.impl_witness) { auto witness_to_impl_id = [&](SemIR::InstId witness_id) { auto table_id = context_.insts() diff --git a/toolchain/check/impl_lookup.cpp b/toolchain/check/impl_lookup.cpp index 0fe1901e6509..89f476cf8384 100644 --- a/toolchain/check/impl_lookup.cpp +++ b/toolchain/check/impl_lookup.cpp @@ -201,7 +201,6 @@ static auto GetInterfacesFromConstantId( context.insts().GetAs(facet_type_inst_id); const auto& facet_type_info = context.facet_types().Get(facet_type_inst.facet_type_id); - // TODO: Get the complete facet type here. auto identified_id = RequireIdentifiedFacetType(context, facet_type_inst, [&] { CARBON_DIAGNOSTIC(ImplLookupInIncompleteFacetType, Error, @@ -324,8 +323,6 @@ static auto LookupImplWitnessInSelfFacetValue( // position of the witness for that interface in `FacetValue`. The // `FacetValue` witnesses are the output of an impl lookup, which finds and // returns witnesses in the same order. - // - // TODO: Get the complete facet type here. auto identified_id = RequireIdentifiedFacetType(context, *facet_type, nullptr); // This should not be possible as FacetValue is constructed by a conversion @@ -803,6 +800,8 @@ static auto CollectCandidateImplsForQuery( llvm::SmallVector candidate_impls; for (auto [id, impl] : context.impls().enumerate()) { + CARBON_CHECK(impl.witness_id.has_value()); + if (final_only && !IsImplEffectivelyFinal(context, impl)) { continue; } @@ -827,14 +826,6 @@ static auto CollectCandidateImplsForQuery( continue; } - // This check comes first to avoid deduction with an invalid impl. We use - // an error value to indicate an error during creation of the impl, such - // as a recursive impl which will cause deduction to recurse infinitely. - if (impl.witness_id == SemIR::ErrorInst::InstId) { - continue; - } - CARBON_CHECK(impl.witness_id.has_value()); - // Build the type structure used for choosing the best the candidate. auto type_structure = BuildTypeStructure(context, impl.self_id, impl.interface); diff --git a/toolchain/check/testdata/impl/error_recovery.carbon b/toolchain/check/testdata/impl/error_recovery.carbon index 630e4dddbff9..61b0353601d1 100644 --- a/toolchain/check/testdata/impl/error_recovery.carbon +++ b/toolchain/check/testdata/impl/error_recovery.carbon @@ -3,8 +3,6 @@ // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // // INCLUDE-FILE: toolchain/testing/testdata/min_prelude/none.carbon -// TODO: Add ranges and switch to "--dump-sem-ir-ranges=only". -// EXTRA-ARGS: --dump-sem-ir-ranges=if-present // // AUTOUPDATE // TIP: To test this file alone, run: @@ -12,35 +10,112 @@ // TIP: To dump output, run: // TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/impl/error_recovery.carbon -// --- fail_fuzz_crash.carbon +// --- fail_runtime_generic_param.carbon +library "[[@TEST_NAME]]"; class C {} interface I {} -// CHECK:STDERR: fail_fuzz_crash.carbon:[[@LINE+4]]:14: error: parameters of generic types must be constant [GenericParamMustBeConstant] -// CHECK:STDERR: impl forall [T: type] C as I { } +//@dump-sem-ir-begin +// CHECK:STDERR: fail_runtime_generic_param.carbon:[[@LINE+4]]:14: error: parameters of generic types must be constant [GenericParamMustBeConstant] +// CHECK:STDERR: impl forall [T: type] C as I {} // CHECK:STDERR: ^~~~~~~ // CHECK:STDERR: -impl forall [T: type] C as I { } +impl forall [T: type] C as I {} +//@dump-sem-ir-end -// CHECK:STDOUT: --- fail_fuzz_crash.carbon +// --- fail_nonfinal_lookup_impl_witness_error_in_import.carbon +library "[[@TEST_NAME]]"; + +interface Z {} +// CHECK:STDERR: fail_nonfinal_lookup_impl_witness_error_in_import.carbon:[[@LINE+4]]:1: error: impl declared but not defined [ImplMissingDefinition] +// CHECK:STDERR: impl forall [T:! type] T as Z; +// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +// CHECK:STDERR: +impl forall [T:! type] T as Z; + +fn F(U:! Z) {} + +//@dump-sem-ir-begin +fn G(T:! type) { + // This makes a LookupImplWitness instruction, but future lookups (evaluation + // of this instruction with a specific) will result in an error since the impl + // is never defined and is left with an error as its witness at the end of the + // file. The lookups should not fail entirely, just result in an error + // witness. + F(T); +} +//@dump-sem-ir-end + +// --- nonfinal_lookup_impl_witness_error_in_import.impl.carbon +impl library "[[@TEST_NAME]]"; + +fn H() { + // The specific here contains errors, but does not fail entirely and crash + // when resolving the LookupImplWitness. + G(()); +} + +// --- fail_nonfinal_lookup_impl_witness_error.carbon +library "[[@TEST_NAME]]"; + +interface Z {} +// CHECK:STDERR: fail_nonfinal_lookup_impl_witness_error.carbon:[[@LINE+4]]:1: error: impl declared but not defined [ImplMissingDefinition] +// CHECK:STDERR: impl forall [T:! type] T as Z; +// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +// CHECK:STDERR: +impl forall [T:! type] T as Z; + +fn F(U:! Z) {} + +//@dump-sem-ir-begin +fn G(T:! type) { + // This makes a LookupImplWitness instruction, but future lookups (evaluation + // of this instruction with a specific) will fail with an error since the impl + // is never defined and is left with an error as its witness at the end of the + // file. The lookups should not fail entirely, just result in an error + // witness. + F(T); +} +//@dump-sem-ir-end + +fn H() { + // The specific here contains errors, but does not fail entirely and crash + // when resolving the LookupImplWitness. + G(()); +} + +// --- fail_final_lookup_impl_witness_error.carbon +library "[[@TEST_NAME]]"; + +interface Z {} +// CHECK:STDERR: fail_final_lookup_impl_witness_error.carbon:[[@LINE+4]]:1: error: impl declared but not defined [ImplMissingDefinition] +// CHECK:STDERR: impl forall [T:! type] T as Z; +// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +// CHECK:STDERR: +impl forall [T:! type] T as Z; + +fn F(U:! Z) {} + +fn G() { + // This impl lookup resolves to a final witness, which poisons any future + // queries. At the end of the file, the poisoned queries are replayed to make + // sure they don't change. However, here it is changed by the impl being + // diagnosed with an error. The poisoning check should handle that gracefully. + //@dump-sem-ir-begin + F(()); + //@dump-sem-ir-end +} + +// CHECK:STDOUT: --- fail_runtime_generic_param.carbon // CHECK:STDOUT: // CHECK:STDOUT: constants { // CHECK:STDOUT: %C: type = class_type @C [concrete] -// CHECK:STDOUT: %empty_struct_type: type = struct_type {} [concrete] -// CHECK:STDOUT: %complete_type: = complete_type_witness %empty_struct_type [concrete] // CHECK:STDOUT: %I.type: type = facet_type <@I> [concrete] -// CHECK:STDOUT: %Self: %I.type = symbolic_binding Self, 0 [symbolic] // CHECK:STDOUT: %I.impl_witness: = impl_witness file.%I.impl_witness_table [concrete] // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: file { -// CHECK:STDOUT: package: = namespace [concrete] { -// CHECK:STDOUT: .C = %C.decl -// CHECK:STDOUT: .I = %I.decl -// CHECK:STDOUT: } -// CHECK:STDOUT: %C.decl: type = class_decl @C [concrete = constants.%C] {} {} -// CHECK:STDOUT: %I.decl: type = interface_decl @I [concrete = constants.%I.type] {} {} // CHECK:STDOUT: impl_decl @C.as.I.impl [concrete] {} { // CHECK:STDOUT: %C.ref: type = name_ref C, file.%C.decl [concrete = constants.%C] // CHECK:STDOUT: %I.ref: type = name_ref I, file.%I.decl [concrete = constants.%I.type] @@ -49,26 +124,138 @@ impl forall [T: type] C as I { } // CHECK:STDOUT: %I.impl_witness: = impl_witness %I.impl_witness_table [concrete = constants.%I.impl_witness] // CHECK:STDOUT: } // CHECK:STDOUT: -// CHECK:STDOUT: interface @I { -// CHECK:STDOUT: %Self: %I.type = symbolic_binding Self, 0 [symbolic = constants.%Self] -// CHECK:STDOUT: -// CHECK:STDOUT: !members: -// CHECK:STDOUT: .Self = %Self -// CHECK:STDOUT: witness = () -// CHECK:STDOUT: -// CHECK:STDOUT: !requires: -// CHECK:STDOUT: } -// CHECK:STDOUT: // CHECK:STDOUT: impl @C.as.I.impl: %C.ref as %I.ref { // CHECK:STDOUT: !members: // CHECK:STDOUT: witness = file.%I.impl_witness // CHECK:STDOUT: } // CHECK:STDOUT: -// CHECK:STDOUT: class @C { -// CHECK:STDOUT: %complete_type: = complete_type_witness constants.%empty_struct_type [concrete = constants.%complete_type] -// CHECK:STDOUT: complete_type_witness = %complete_type +// CHECK:STDOUT: --- fail_nonfinal_lookup_impl_witness_error_in_import.carbon // CHECK:STDOUT: -// CHECK:STDOUT: !members: -// CHECK:STDOUT: .Self = constants.%C +// CHECK:STDOUT: constants { +// CHECK:STDOUT: %Z.type: type = facet_type <@Z> [concrete] +// CHECK:STDOUT: %T: type = symbolic_binding T, 0 [symbolic] +// CHECK:STDOUT: %pattern_type.98f: type = pattern_type type [concrete] +// CHECK:STDOUT: %F.type: type = fn_type @F [concrete] +// CHECK:STDOUT: %empty_tuple.type: type = tuple_type () [concrete] +// CHECK:STDOUT: %F: %F.type = struct_value () [concrete] +// CHECK:STDOUT: %G.type: type = fn_type @G [concrete] +// CHECK:STDOUT: %G: %G.type = struct_value () [concrete] +// CHECK:STDOUT: %Z.lookup_impl_witness: = lookup_impl_witness %T, @Z [symbolic] +// CHECK:STDOUT: %Z.facet: %Z.type = facet_value %T, (%Z.lookup_impl_witness) [symbolic] +// CHECK:STDOUT: %F.specific_fn: = specific_function %F, @F(%Z.facet) [symbolic] +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: file { +// CHECK:STDOUT: %G.decl: %G.type = fn_decl @G [concrete = constants.%G] { +// CHECK:STDOUT: %T.patt: %pattern_type.98f = symbolic_binding_pattern T, 0 [concrete] +// CHECK:STDOUT: } { +// CHECK:STDOUT: +// CHECK:STDOUT: %T.loc13_6.2: type = symbolic_binding T, 0 [symbolic = %T.loc13_6.1 (constants.%T)] +// CHECK:STDOUT: } +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: generic fn @G(%T.loc13_6.2: type) { +// CHECK:STDOUT: %T.loc13_6.1: type = symbolic_binding T, 0 [symbolic = %T.loc13_6.1 (constants.%T)] +// CHECK:STDOUT: +// CHECK:STDOUT: !definition: +// CHECK:STDOUT: %Z.lookup_impl_witness: = lookup_impl_witness %T.loc13_6.1, @Z [symbolic = %Z.lookup_impl_witness (constants.%Z.lookup_impl_witness)] +// CHECK:STDOUT: %Z.facet.loc19_6.2: %Z.type = facet_value %T.loc13_6.1, (%Z.lookup_impl_witness) [symbolic = %Z.facet.loc19_6.2 (constants.%Z.facet)] +// CHECK:STDOUT: %F.specific_fn.loc19_3.2: = specific_function constants.%F, @F(%Z.facet.loc19_6.2) [symbolic = %F.specific_fn.loc19_3.2 (constants.%F.specific_fn)] +// CHECK:STDOUT: +// CHECK:STDOUT: fn() { +// CHECK:STDOUT: !entry: +// CHECK:STDOUT: %F.ref: %F.type = name_ref F, file.%F.decl [concrete = constants.%F] +// CHECK:STDOUT: %T.ref: type = name_ref T, %T.loc13_6.2 [symbolic = %T.loc13_6.1 (constants.%T)] +// CHECK:STDOUT: %Z.facet.loc19_6.1: %Z.type = facet_value %T.ref, (constants.%Z.lookup_impl_witness) [symbolic = %Z.facet.loc19_6.2 (constants.%Z.facet)] +// CHECK:STDOUT: %.loc19: %Z.type = converted %T.ref, %Z.facet.loc19_6.1 [symbolic = %Z.facet.loc19_6.2 (constants.%Z.facet)] +// CHECK:STDOUT: %F.specific_fn.loc19_3.1: = specific_function %F.ref, @F(constants.%Z.facet) [symbolic = %F.specific_fn.loc19_3.2 (constants.%F.specific_fn)] +// CHECK:STDOUT: %F.call: init %empty_tuple.type = call %F.specific_fn.loc19_3.1() +// CHECK:STDOUT: return +// CHECK:STDOUT: } +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: specific @G(constants.%T) { +// CHECK:STDOUT: %T.loc13_6.1 => constants.%T +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: --- fail_nonfinal_lookup_impl_witness_error.carbon +// CHECK:STDOUT: +// CHECK:STDOUT: constants { +// CHECK:STDOUT: %Z.type: type = facet_type <@Z> [concrete] +// CHECK:STDOUT: %T: type = symbolic_binding T, 0 [symbolic] +// CHECK:STDOUT: %pattern_type.98f: type = pattern_type type [concrete] +// CHECK:STDOUT: %F.type: type = fn_type @F [concrete] +// CHECK:STDOUT: %empty_tuple.type: type = tuple_type () [concrete] +// CHECK:STDOUT: %F: %F.type = struct_value () [concrete] +// CHECK:STDOUT: %G.type: type = fn_type @G [concrete] +// CHECK:STDOUT: %G: %G.type = struct_value () [concrete] +// CHECK:STDOUT: %Z.lookup_impl_witness: = lookup_impl_witness %T, @Z [symbolic] +// CHECK:STDOUT: %Z.facet: %Z.type = facet_value %T, (%Z.lookup_impl_witness) [symbolic] +// CHECK:STDOUT: %F.specific_fn: = specific_function %F, @F(%Z.facet) [symbolic] +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: file { +// CHECK:STDOUT: %G.decl: %G.type = fn_decl @G [concrete = constants.%G] { +// CHECK:STDOUT: %T.patt: %pattern_type.98f = symbolic_binding_pattern T, 0 [concrete] +// CHECK:STDOUT: } { +// CHECK:STDOUT: +// CHECK:STDOUT: %T.loc13_6.2: type = symbolic_binding T, 0 [symbolic = %T.loc13_6.1 (constants.%T)] +// CHECK:STDOUT: } +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: generic fn @G(%T.loc13_6.2: type) { +// CHECK:STDOUT: %T.loc13_6.1: type = symbolic_binding T, 0 [symbolic = %T.loc13_6.1 (constants.%T)] +// CHECK:STDOUT: +// CHECK:STDOUT: !definition: +// CHECK:STDOUT: %Z.lookup_impl_witness: = lookup_impl_witness %T.loc13_6.1, @Z [symbolic = %Z.lookup_impl_witness (constants.%Z.lookup_impl_witness)] +// CHECK:STDOUT: %Z.facet.loc19_6.2: %Z.type = facet_value %T.loc13_6.1, (%Z.lookup_impl_witness) [symbolic = %Z.facet.loc19_6.2 (constants.%Z.facet)] +// CHECK:STDOUT: %F.specific_fn.loc19_3.2: = specific_function constants.%F, @F(%Z.facet.loc19_6.2) [symbolic = %F.specific_fn.loc19_3.2 (constants.%F.specific_fn)] +// CHECK:STDOUT: +// CHECK:STDOUT: fn() { +// CHECK:STDOUT: !entry: +// CHECK:STDOUT: %F.ref: %F.type = name_ref F, file.%F.decl [concrete = constants.%F] +// CHECK:STDOUT: %T.ref: type = name_ref T, %T.loc13_6.2 [symbolic = %T.loc13_6.1 (constants.%T)] +// CHECK:STDOUT: %Z.facet.loc19_6.1: %Z.type = facet_value %T.ref, (constants.%Z.lookup_impl_witness) [symbolic = %Z.facet.loc19_6.2 (constants.%Z.facet)] +// CHECK:STDOUT: %.loc19: %Z.type = converted %T.ref, %Z.facet.loc19_6.1 [symbolic = %Z.facet.loc19_6.2 (constants.%Z.facet)] +// CHECK:STDOUT: %F.specific_fn.loc19_3.1: = specific_function %F.ref, @F(constants.%Z.facet) [symbolic = %F.specific_fn.loc19_3.2 (constants.%F.specific_fn)] +// CHECK:STDOUT: %F.call: init %empty_tuple.type = call %F.specific_fn.loc19_3.1() +// CHECK:STDOUT: return +// CHECK:STDOUT: } +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: specific @G(constants.%T) { +// CHECK:STDOUT: %T.loc13_6.1 => constants.%T +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: specific @G(constants.%empty_tuple.type) { +// CHECK:STDOUT: %T.loc13_6.1 => constants.%empty_tuple.type +// CHECK:STDOUT: +// CHECK:STDOUT: !definition: +// CHECK:STDOUT: %Z.lookup_impl_witness => +// CHECK:STDOUT: %Z.facet.loc19_6.2 => +// CHECK:STDOUT: %F.specific_fn.loc19_3.2 => +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: --- fail_final_lookup_impl_witness_error.carbon +// CHECK:STDOUT: +// CHECK:STDOUT: constants { +// CHECK:STDOUT: %Z.type: type = facet_type <@Z> [concrete] +// CHECK:STDOUT: %F.type: type = fn_type @F [concrete] +// CHECK:STDOUT: %empty_tuple.type: type = tuple_type () [concrete] +// CHECK:STDOUT: %F: %F.type = struct_value () [concrete] +// CHECK:STDOUT: %Z.impl_witness.b07: = impl_witness file.%Z.impl_witness_table, @T.as.Z.impl(%empty_tuple.type) [concrete] +// CHECK:STDOUT: %Z.facet: %Z.type = facet_value %empty_tuple.type, (%Z.impl_witness.b07) [concrete] +// CHECK:STDOUT: %F.specific_fn: = specific_function %F, @F(%Z.facet) [concrete] +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: fn @G() { +// CHECK:STDOUT: !entry: +// CHECK:STDOUT: %F.ref: %F.type = name_ref F, file.%F.decl [concrete = constants.%F] +// CHECK:STDOUT: %.loc18_6: %empty_tuple.type = tuple_literal () +// CHECK:STDOUT: %Z.facet: %Z.type = facet_value constants.%empty_tuple.type, (constants.%Z.impl_witness.b07) [concrete = constants.%Z.facet] +// CHECK:STDOUT: %.loc18_7: %Z.type = converted %.loc18_6, %Z.facet [concrete = constants.%Z.facet] +// CHECK:STDOUT: %F.specific_fn: = specific_function %F.ref, @F(constants.%Z.facet) [concrete = constants.%F.specific_fn] +// CHECK:STDOUT: %F.call: init %empty_tuple.type = call %F.specific_fn() +// CHECK:STDOUT: // CHECK:STDOUT: } // CHECK:STDOUT: diff --git a/toolchain/sem_ir/type_iterator.h b/toolchain/sem_ir/type_iterator.h index 20fa0e41288a..64ca63bf813a 100644 --- a/toolchain/sem_ir/type_iterator.h +++ b/toolchain/sem_ir/type_iterator.h @@ -40,7 +40,8 @@ class TypeIterator { // The iterator will visit things in the reverse order that they are added. auto Add(InstId inst_id) -> void { auto type_id = sem_ir_->insts().Get(inst_id).type_id(); - CARBON_CHECK(sem_ir_->types().IsFacetType(type_id), + CARBON_CHECK(sem_ir_->types().IsFacetType(type_id) || + type_id == SemIR::ErrorInst::TypeId, "Type {0} of type inst is not a facet type", sem_ir_->types().GetAsInst(type_id).kind()); PushInstId(inst_id);