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);