diff --git a/toolchain/check/handle_impl.cpp b/toolchain/check/handle_impl.cpp index 08f6f1998d63..21f314dd2fbe 100644 --- a/toolchain/check/handle_impl.cpp +++ b/toolchain/check/handle_impl.cpp @@ -335,6 +335,8 @@ auto HandleParseNode(Context& context, Parse::ImplDefinitionStartId node_id) auto [impl_id, impl_decl_id] = BuildImplDecl(context, node_id); auto& impl = context.impls().Get(impl_id); + CheckRequireDeclsSatisfied(context, node_id, impl); + CARBON_CHECK(!impl.has_definition_started()); impl.definition_id = impl_decl_id; impl.scope_id = @@ -346,7 +348,6 @@ auto HandleParseNode(Context& context, Parse::ImplDefinitionStartId node_id) context.generics().GetSelfSpecific(impl.generic_id)); StartGenericDefinition(context, impl.generic_id); ImplWitnessStartDefinition(context, impl); - CheckRequireDeclsSatisfied(context, impl); context.inst_block_stack().Push(); context.node_stack().Push(node_id, impl_id); diff --git a/toolchain/check/impl.cpp b/toolchain/check/impl.cpp index bbaac6b951a5..351f1166b92d 100644 --- a/toolchain/check/impl.cpp +++ b/toolchain/check/impl.cpp @@ -663,12 +663,21 @@ auto FinishImplWitness(Context& context, const SemIR::Impl& impl) -> void { // TODO: Diagnose if any declarations in the impl are not in used_decl_ids. } -auto CheckRequireDeclsSatisfied(Context& context, SemIR::Impl& impl) -> void { +auto CheckRequireDeclsSatisfied(Context& context, SemIR::LocId loc_id, + SemIR::Impl& impl) -> void { if (impl.witness_id == SemIR::ErrorInst::InstId) { return; } const auto& interface = context.interfaces().Get(impl.interface.interface_id); + if (!interface.is_complete()) { + // This will be diagnosed later. We check for required decls before starting + // the definition to avoid inserting these lookups into the definition, as + // the lookups can end up looking for the impl being defined, which creates + // a cycle. + return; + } + auto require_ids = context.require_impls_blocks().Get(interface.require_impls_block_id); if (require_ids.empty()) { @@ -690,8 +699,7 @@ auto CheckRequireDeclsSatisfied(Context& context, SemIR::Impl& impl) -> void { context, require_specific, require.facet_type_inst_id); auto result = - LookupImplWitness(context, SemIR::LocId(impl.latest_decl_id()), - self_const_id, facet_type_const_id); + LookupImplWitness(context, loc_id, self_const_id, facet_type_const_id); // TODO: If the facet type contains 2 interfaces, and one is not `impl`ed, // it would be nice to diagnose which one was not `impl`ed, but that // requires LookupImplWitness to return a partial result, or take a @@ -708,7 +716,7 @@ auto CheckRequireDeclsSatisfied(Context& context, SemIR::Impl& impl) -> void { SemIR::SpecificInterface, SemIR::TypeId, SemIR::FacetTypeId); context.emitter().Emit( - impl.latest_decl_id(), RequireImplsNotImplemented, impl.interface, + loc_id, RequireImplsNotImplemented, impl.interface, context.types().GetTypeIdForTypeConstantId(self_const_id), context.insts() .GetAs(facet_type_inst_id) diff --git a/toolchain/check/impl.h b/toolchain/check/impl.h index 040061c521c7..062daec93673 100644 --- a/toolchain/check/impl.h +++ b/toolchain/check/impl.h @@ -64,7 +64,8 @@ auto FinishImplWitness(Context& context, const SemIR::Impl& impl_id) -> void; // Checks that any `require` declarations in the interface being implemented by // `impl` are satisfied. Otherwise, a diagnostic is issued and the `impl` is // made invalid. -auto CheckRequireDeclsSatisfied(Context& context, SemIR::Impl& impl) -> void; +auto CheckRequireDeclsSatisfied(Context& context, SemIR::LocId loc_id, + SemIR::Impl& impl) -> void; // Sets all unset members of the witness for `impl` to the error instruction and // sets the witness id in the `Impl` to an error. diff --git a/toolchain/check/testdata/facet/require_satisified.carbon b/toolchain/check/testdata/facet/require_satisified.carbon index b84af123e80a..0350f09740c0 100644 --- a/toolchain/check/testdata/facet/require_satisified.carbon +++ b/toolchain/check/testdata/facet/require_satisified.carbon @@ -2,7 +2,7 @@ // Exceptions. See /LICENSE for license information. // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // -// INCLUDE-FILE: toolchain/testing/testdata/min_prelude/convert.carbon +// INCLUDE-FILE: toolchain/testing/testdata/min_prelude/int.carbon // // AUTOUPDATE // TIP: To test this file alone, run: @@ -184,3 +184,75 @@ impl C as Z(()); impl () as Y(Self) {} impl C as Z(()) {} + +// --- impl_requires_itself_cycle.carbon +library "[[@TEST_NAME]]"; + +interface A { fn AA(); } +interface B { + require impls A; +} +interface C { + require impls B; +} + +impl forall [T:! B] T as A { fn AA() {} } + +// When we get to the definition we check that anything B requires is satisfied. +// - The interface B requires A, so we must check T impls A. +// - The impl for A requires T impls B. +// - This impl is what provides T impls B. +impl forall [T:! C] T as B {} + +fn F(T:! C) { + // If things go wrong, we find the impl `T as B`, but inside its definition is + // a lookup for `T as A`, which (through deducing `T`) includes a lookup for + // `T as B`. This creates a cycle of evaluating `T as B` recursively. + // + // This was solved by doing the check for requirements of `B` outside the + // definition of `T as B`. + // https://discord.com/channels/655572317891461132/941071822756143115/1463189087598022861 + T.(A.AA)(); +} + +// --- fail_impl_requires_itself_cycle_with_monomorphization_error.carbon +library "[[@TEST_NAME]]"; + +class W(T:! type) { + adapt {}; +} + +interface A(N:! Core.IntLiteral()) { + // CHECK:STDERR: fail_impl_requires_itself_cycle_with_monomorphization_error.carbon:[[@LINE+3]]:26: error: array bound of -1 is negative [ArrayBoundNegative] + // CHECK:STDERR: fn AA() -> W(array((), N)); + // CHECK:STDERR: ^ + fn AA() -> W(array((), N)); +} +interface B(N:! Core.IntLiteral()) { + require impls A(N); +} +interface C(N:! Core.IntLiteral()) { + require impls B(N); +} + +impl forall [N:! Core.IntLiteral(), T:! B(N)] T as A(N) { + fn AA() -> W(array((), N)) { return {} as W(array((), N)); } +} + +impl forall [N:! Core.IntLiteral(), T:! C(N)] T as B(N) { + // The definition here does not contain the lookups verifying that C(N) impls + // `A(N)`, so they do not get re-evaluated for a specific `N`. That doesn't + // prevent us from producing a reasonable diagnostic when that `N` causes an + // error in the specific use of this impl, which we use to get from `C(N)` to + // `A(N)` (in the deduction of `T` in the impl as `A(N)`). The error just + // happens where we use `N` in `A(N)` instead of inside the verification that + // `T as B(N)` implies `T as A(N)`. +} + +fn F(T:! C(-1)) { + // CHECK:STDERR: fail_impl_requires_itself_cycle_with_monomorphization_error.carbon:[[@LINE+4]]:3: note: in `AA()` used here [ResolvingSpecificHere] + // CHECK:STDERR: T.(A(-1).AA)(); + // CHECK:STDERR: ^~~~~~~~~~~~~~ + // CHECK:STDERR: + T.(A(-1).AA)(); +} diff --git a/toolchain/check/testdata/impl/import_generic.carbon b/toolchain/check/testdata/impl/import_generic.carbon index accd2ca627c5..fb5714f9b467 100644 --- a/toolchain/check/testdata/impl/import_generic.carbon +++ b/toolchain/check/testdata/impl/import_generic.carbon @@ -325,10 +325,10 @@ impl forall [T:! type] D as N(T*) {} // CHECK:STDOUT: %require_complete: = require_complete_type %I.type.1ab [symbolic] // CHECK:STDOUT: %J.type.d6f: type = facet_type <@J, @J(%empty_struct_type)> [concrete] // CHECK:STDOUT: %J.impl_witness: = impl_witness @C.as.J.impl.%J.impl_witness_table [concrete] -// CHECK:STDOUT: %Self.9e4: %J.type.d6f = symbolic_binding Self, 1 [symbolic] -// CHECK:STDOUT: %complete_type.5b1: = complete_type_witness %I.type.ab5 [concrete] // CHECK:STDOUT: %type: type = facet_type [concrete] // CHECK:STDOUT: %C.type.facet: %type = facet_value %C, () [concrete] +// CHECK:STDOUT: %Self.9e4: %J.type.d6f = symbolic_binding Self, 1 [symbolic] +// CHECK:STDOUT: %complete_type.5b1: = complete_type_witness %I.type.ab5 [concrete] // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: imports {