From eea1e5837692a129afd768acfe8f64f57a9279bc Mon Sep 17 00:00:00 2001 From: Dana Jansens Date: Mon, 11 May 2026 10:17:18 -0400 Subject: [PATCH] Correctly handle ImplWitnessAccess in impl lookup (#7181) We were treating ImplWitnessAccess as a concrete type, but that is incorrect if its accessing a symbolic type value. This results in concrete impl lookup queries failing to match a generic impl that is built with a symbolic ImplWitnessAccess in its type structure, when the query does not have the equivalent ImplWitnessAccess in its own type structure. We need to look in the top level facet being accessed through ImplWitnessAccess for witnesses, such as in `T:! Z where .Z1 impls Y` where `T` provides the witness for `T.Z1 as Y`. But we also need to look in the facet type of the ImplWitnessAccess for witnesses, such as in `T:! Z` for `interface Z { let Z1:! Y }`, where `T.Z1` provides the witness for `T.Z1 as Y`. To support that we give TypeIterator an iteration step for ImplWitnessAccess before recursing into it, like we do for FacetValue. While doing this, we make TypeIterator more recursive, by making less special casing around the step from one inst into the next. Instead of eagerly finding a SymbolicType, we consistently recurse back into the big switch statement and have it decide the next iteration step. This allows it to recurse into instructions like ImplWitnessAccess and FacetValue in a consistent manner. --- toolchain/check/impl_lookup.cpp | 20 ++- .../facet/validate_impl_constraints.carbon | 34 ++++ .../check/testdata/impl/lookup/access.carbon | 54 ++++++ toolchain/check/type_structure.cpp | 8 +- toolchain/sem_ir/type_iterator.cpp | 156 ++++++++---------- toolchain/sem_ir/type_iterator.h | 64 ++++--- 6 files changed, 208 insertions(+), 128 deletions(-) create mode 100644 toolchain/check/testdata/impl/lookup/access.carbon diff --git a/toolchain/check/impl_lookup.cpp b/toolchain/check/impl_lookup.cpp index fb0f64074709..2e1e8c424a17 100644 --- a/toolchain/check/impl_lookup.cpp +++ b/toolchain/check/impl_lookup.cpp @@ -391,11 +391,21 @@ static auto CollectFacetWitnessSources( done = true; break; } - case CARBON_KIND(Step::FacetValue value): { - // We want to store FacetValues since they come with final witnesses, - // regardless of whether they internally hold a concrete type or a - // symbolic one (with non-final witnesses of its own). - push_facet(value.facet_value_inst_id, allow_partially_identified); + case CARBON_KIND(Step::TypeWrapper wrapper): { + switch (wrapper.kind) { + case Step::TypeWrapper::FacetValue: + // We want to store FacetValues since they come with final + // witnesses, regardless of whether they internally hold a + // concrete type or a symbolic one (with non-final witnesses of + // its own). + push_facet(wrapper.inst_id, allow_partially_identified); + break; + case Step::TypeWrapper::ImplWitnessAccess: + // We want to store ImplWitnessAccess because the associated + // constant may be a facet with witnesses. + push_facet(wrapper.inst_id, allow_partially_identified); + break; + } break; } case CARBON_KIND(Step::SymbolicType symbolic): { diff --git a/toolchain/check/testdata/facet/validate_impl_constraints.carbon b/toolchain/check/testdata/facet/validate_impl_constraints.carbon index a298b83bf8fc..7bb4063f244e 100644 --- a/toolchain/check/testdata/facet/validate_impl_constraints.carbon +++ b/toolchain/check/testdata/facet/validate_impl_constraints.carbon @@ -418,3 +418,37 @@ fn F(B:! W(E), C:! type where E impls V(.Self)) { E as (V(C) where D(B) impls X(.Self)); E as (V(C) where D(B) impls X(E)); } + +// --- facet_has_witness_for_impl_witness_access.carbon +library "[[@TEST_NAME]]"; + +interface X {} +interface Y {} +interface Z { + // A type type, which has no witnesses. + let Z1:! type; + // A facet type, but not one that provides a witness for `.Z2 as Y`. + let Z2:! type where .Self impls X; +} +fn F(T:! Z where .Z1 impls Y and .Z2 impls Y) { + // `T.Z1` is a `type` so this query self is ImplWitnessAccess(Z1). + T.Z1 as Y; + // `T.Z2` has a facet type so this query self is FacetAccessType(ImplWitnessAccess(Z2)). + T.Z2 as Y; +} + +// --- impl_witness_access_has_access.carbon +library "[[@TEST_NAME]]"; + +class C(T:! type) {} + +interface Y {} +interface Z { + // Note that the facet type is not an exact type match for `Y` so that we + // cause an actual impl lookup to happen when we convert `.Z1` to `Y`. + let Z1:! type where .Self impls Y and C(.Self) impls Y; +} +fn F(T:! Z) { + T.Z1 as Y; + C(T.Z1) as Y; +} diff --git a/toolchain/check/testdata/impl/lookup/access.carbon b/toolchain/check/testdata/impl/lookup/access.carbon new file mode 100644 index 000000000000..97997b91601f --- /dev/null +++ b/toolchain/check/testdata/impl/lookup/access.carbon @@ -0,0 +1,54 @@ +// 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/none.carbon +// +// AUTOUPDATE +// TIP: To test this file alone, run: +// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/impl/lookup/access.carbon +// TIP: To dump output, run: +// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/impl/lookup/access.carbon + +// --- impl_witness_access_in_impl_type_structure.carbon +library "[[@TEST_NAME]]"; + +interface X(T:! type) {} +interface Y { + let Y1:! type; +} +interface Z { + let Z1:! type; +} + +// There's 2 ImplWitnessAccess instructions in the type, but they together +// resolve to a symbolic type value, so the type structure is: `?C(?)` +impl forall [U:! Z where .Z1 impls Y] U as X(U.Z1.(Y.Y1)) {} + +class C { + impl as Y where .Y1 = {} {} +} + +fn F(V:! Z where .Z1 = C) { + // The type stucture is `?C(C)` which will match the impl's less specific + // `?C(?)`, then the impl will deduce the parameter of `X` to be `{}` from the + // type of `V`. This would fail if ImplWitnessAccess instructions were treated + // as Concrete in the type structure, since the impl would have a different + // concrete value (an ImplWitnessAccess) than the query (a StructValue). + V as X({}); +} + +fn G(V:! Z where .Z1 impls Y) { + // The type structure is `?C(?)`, also built from ImplWitnessAccess insts, + // which will match the impl's `?C(?)`. + V as X(V.(Z.Z1).(Y.Y1)); +} + +// TODO: This creates an infinite loop in `.Self` substitution. +// fn H(U:! type, V:! Z where .Z1 impls (Y where .Y1 = U)) { +// // The type structure is `?C(?)`, without using an ImplWitnessAccess, which +// // will match the impl's `?C(?)`. This would fail if ImplWitnessAccess +// // instructions were treated as Concrete in the type structure, since the +// // impl's type structure would be more specific than the query's. +// V as X(U); +// } diff --git a/toolchain/check/type_structure.cpp b/toolchain/check/type_structure.cpp index 496f87c420a6..3106fa7b2569 100644 --- a/toolchain/check/type_structure.cpp +++ b/toolchain/check/type_structure.cpp @@ -229,14 +229,14 @@ auto TypeStructureBuilder::Build(SemIR::TypeIterator type_iter) AppendStructuralSymbolic(); break; } - case CARBON_KIND(Step::FacetValue _): { - // Ignored, as it may be concrete or symbolic. We will recurse into it. - break; - } case CARBON_KIND(Step::TemplateType _): { AppendStructuralSymbolic(); break; } + case CARBON_KIND(Step::TypeWrapper _): { + // Ignored, as it may be concrete or symbolic. We will recurse into it. + break; + } case CARBON_KIND(Step::ConcreteValue value): { AppendStructuralConcrete( context_->constant_values().Get(value.inst_id)); diff --git a/toolchain/sem_ir/type_iterator.cpp b/toolchain/sem_ir/type_iterator.cpp index 035f500bac80..75b6cdfc0880 100644 --- a/toolchain/sem_ir/type_iterator.cpp +++ b/toolchain/sem_ir/type_iterator.cpp @@ -81,16 +81,8 @@ auto TypeIterator::Next() -> Step { case CARBON_KIND(SymbolicNonTypeValue value): { return Step::SymbolicValue{.inst_id = value.inst_id}; } - case CARBON_KIND(FacetValueItem facet_value): { - return Step::FacetValue{.facet_value_inst_id = - facet_value.facet_value_inst_id}; - } - case CARBON_KIND(SymbolicType symbolic): { - return Step::SymbolicType{.entity_name_id = symbolic.entity_name_id, - .facet = symbolic.facet}; - } - case CARBON_KIND(TypeId type_id): { - if (auto step = ProcessTypeId(type_id)) { + case CARBON_KIND(TypeValue value): { + if (auto step = ProcessType(value.inst_id)) { return *step; } } @@ -100,8 +92,7 @@ auto TypeIterator::Next() -> Step { return Step::Done(); } -auto TypeIterator::ProcessTypeId(TypeId type_id) -> std::optional { - auto inst_id = sem_ir_->types().GetTypeInstId(type_id); +auto TypeIterator::ProcessType(InstId inst_id) -> std::optional { auto inst = sem_ir_->insts().Get(inst_id); // TODO: This categorization should mostly be driven by information in the // inst kind. @@ -122,17 +113,6 @@ auto TypeIterator::ProcessTypeId(TypeId type_id) -> std::optional { return Step::TemplateType(); } - case CARBON_KIND(FacetAccessType access): { - auto entity_name_id = SemIR::EntityNameId::None; - if (auto facet_value = sem_ir_->insts().TryGetAs( - access.facet_value_inst_id)) { - entity_name_id = facet_value->entity_name_id; - } - - return Step::SymbolicType{.entity_name_id = entity_name_id, - .facet = access.facet_value_inst_id}; - } - case TupleAccess::Kind: { // Tuple access of a concrete value would have evaluated to the accessed // value, so we only see TupleAccess in a type when it's a symbolic value. @@ -157,7 +137,6 @@ auto TypeIterator::ProcessTypeId(TypeId type_id) -> std::optional { case GenericClassType::Kind: case GenericInterfaceType::Kind: case GenericNamedConstraintType::Kind: - case ImplWitnessAccess::Kind: case IntLiteralType::Kind: case NamespaceType::Kind: case RequireSpecificDefinitionType::Kind: @@ -165,13 +144,15 @@ auto TypeIterator::ProcessTypeId(TypeId type_id) -> std::optional { case UnboundElementType::Kind: case VtableType::Kind: case WitnessType::Kind: { - return Step::ConcreteType{.type_id = type_id}; + return Step::ConcreteType{ + .type_id = sem_ir_->types().GetTypeIdForTypeInstId(inst_id)}; } case CARBON_KIND(IntType int_type): { Push(EndType()); PushArgs({int_type.bit_width_id}); - return Step::IntStart{.type_id = type_id}; + return Step::IntStart{ + .type_id = sem_ir_->types().GetTypeIdForTypeInstId(inst_id)}; } // ==== Aggregate types ==== @@ -180,18 +161,21 @@ auto TypeIterator::ProcessTypeId(TypeId type_id) -> std::optional { Push(EndType()); PushInstId(array_type.element_type_inst_id); PushInstId(array_type.bound_id); - return Step::ArrayStart{.type_id = type_id}; + return Step::ArrayStart{ + .type_id = sem_ir_->types().GetTypeIdForTypeInstId(inst_id)}; } case CARBON_KIND(ClassType class_type): { auto args = GetSpecificArgs(class_type.specific_id); if (args.empty()) { return Step::ClassStartOnly{ - {.class_id = class_type.class_id, .type_id = type_id}}; + {.class_id = class_type.class_id, + .type_id = sem_ir_->types().GetTypeIdForTypeInstId(inst_id)}}; } else { Push(EndType()); PushArgs(args); - return Step::ClassStart{.class_id = class_type.class_id, - .type_id = type_id}; + return Step::ClassStart{ + .class_id = class_type.class_id, + .type_id = sem_ir_->types().GetTypeIdForTypeInstId(inst_id)}; } } case CARBON_KIND(ConstType const_type): { @@ -200,7 +184,7 @@ auto TypeIterator::ProcessTypeId(TypeId type_id) -> std::optional { return Step::ConstStart(); } case CARBON_KIND(ImplWitnessAssociatedConstant assoc): { - Push(assoc.type_id); + PushTypeId(assoc.type_id); return std::nullopt; } case CARBON_KIND(MaybeUnformedType maybe_unformed_type): { @@ -222,75 +206,78 @@ auto TypeIterator::ProcessTypeId(TypeId type_id) -> std::optional { auto inner_types = sem_ir_->inst_blocks().Get(tuple_type.type_elements_id); if (inner_types.empty()) { - return Step::TupleStartOnly{{.type_id = type_id}}; + return Step::TupleStartOnly{ + {.type_id = sem_ir_->types().GetTypeIdForTypeInstId(inst_id)}}; } else { Push(EndType()); PushArgs(sem_ir_->inst_blocks().Get(tuple_type.type_elements_id)); - return Step::TupleStart{.type_id = type_id}; + return Step::TupleStart{ + .type_id = sem_ir_->types().GetTypeIdForTypeInstId(inst_id)}; } } case CARBON_KIND(StructType struct_type): { auto fields = sem_ir_->struct_type_fields().Get(struct_type.fields_id); if (fields.empty()) { - return Step::StructStartOnly{{.type_id = type_id}}; + return Step::StructStartOnly{ + {.type_id = sem_ir_->types().GetTypeIdForTypeInstId(inst_id)}}; } else { Push(EndType()); for (const auto& field : llvm::reverse(fields)) { Push(StructFieldName{.name_id = field.name_id}); PushInstId(field.type_inst_id); } - return Step::StructStart{.type_id = type_id}; + return Step::StructStart{ + .type_id = sem_ir_->types().GetTypeIdForTypeInstId(inst_id)}; } } + // ==== Dependent instructions ==== + + case CARBON_KIND(FacetAccessType access): { + if (sem_ir_->constant_values().Get(inst_id).is_concrete()) { + return Step::ConcreteType{ + .type_id = sem_ir_->types().GetTypeIdForTypeInstId(inst_id)}; + } + PushInstId(access.facet_value_inst_id); + return std::nullopt; + } + + case CARBON_KIND(FacetValue value): { + // We return FacetValues as a separate iterative step, then also recurse + // into them. + PushInstId(value.type_inst_id); + return Step::TypeWrapper{.kind = Step::TypeWrapper::FacetValue, + .inst_id = inst_id}; + } + + case CARBON_KIND(ImplWitnessAccess access): { + // We return FacetValues as a separate iterative step, then also recurse + // into the the self value being accessed. + // + // Witness access of a concrete value would have evaluated to the accessed + // value, so we only see ImplWitnessAccess in a type when it's a symbolic + // value, which implies it contains a LookupImplWitness. + CARBON_CHECK(sem_ir_->constant_values().Get(inst_id).is_symbolic()); + auto witness = + sem_ir_->insts().GetAs(access.witness_id); + // Recurse into symbolic ImplWitnessAccess, replacing it with the self + // value for the iteration. If there are nested accesses, this replaces + // them all with the root self. + PushInstId(witness.query_self_inst_id); + return Step::TypeWrapper{.kind = Step::TypeWrapper::ImplWitnessAccess, + .inst_id = inst_id}; + } + case ErrorInst::Kind: return Step::Error(); default: // TODO: Rearrange this so that missing instruction kinds are detected // at compile-time not runtime. - CARBON_FATAL("Unhandled type instruction {0}", inst_id); + CARBON_FATAL("Unhandled type instruction {0}", inst); } } -auto TypeIterator::TryGetInstIdAsTypeId(InstId inst_id) const - -> std::variant { - if (auto facet_value = sem_ir_->insts().TryGetAs(inst_id)) { - inst_id = facet_value->type_inst_id; - } - - auto type_id_of_inst_id = sem_ir_->insts().Get(inst_id).type_id(); - // All instructions of type FacetType are symbolic except for FacetValue: - // - In non-generic code, values of type FacetType are only created through - // conversion to a FacetType (e.g. `Class as Iface`), which produces a - // non-symbolic FacetValue. - // - In generic code, binding values of type FacetType are symbolic as they - // refer to an unknown type. Non-binding values would be FacetValues like - // in non-generic code, but would be symbolic as well. - // - In specifics of generic code, when deducing a value for a symbolic - // binding of type FacetType, we always produce a FacetValue (which may or - // may not itself be symbolic) through conversion. - // - // FacetValues are handled earlier by getting the type instruction from - // them. That type instruction is never of type FacetType. If it refers to a - // FacetType it does so through a FacetAccessType, which is of type TypeType - // and thus does not match here. - if (auto facet_type = - sem_ir_->types().TryGetAs(type_id_of_inst_id)) { - auto entity_name_id = SemIR::EntityNameId::None; - if (auto bind = - sem_ir_->insts().TryGetAs(inst_id)) { - entity_name_id = bind->entity_name_id; - } - return SymbolicType{.entity_name_id = entity_name_id, .facet = inst_id}; - } - // Non-type values are concrete, only types are symbolic. - if (type_id_of_inst_id != TypeType::TypeId) { - return TypeId::None; - } - return sem_ir_->types().GetTypeIdForTypeInstId(inst_id); -} - auto TypeIterator::GetSpecificArgs(SpecificId specific_id) const -> llvm::ArrayRef { if (specific_id == SpecificId::None) { @@ -310,18 +297,13 @@ auto TypeIterator::PushArgs(llvm::ArrayRef args) -> void { // Push an instruction's type value into the work queue, or a marker if the // instruction has a symbolic value. auto TypeIterator::PushInstId(InstId inst_id) -> void { - // We push FacetValues as a separate work step, then also recurse into them - // below. - if (sem_ir_->insts().Is(inst_id)) { - Push(FacetValueItem{.facet_value_inst_id = inst_id}); - } + // Work with canonical instructions only. Types always have a constant value. + // Do this here instead of in Add(InstId) to also handle the user providing a + // non-canonical input through other Add() methods. + inst_id = sem_ir_->constant_values().GetConstantInstId(inst_id); - auto maybe_type_id = TryGetInstIdAsTypeId(inst_id); - if (std::holds_alternative(maybe_type_id)) { - Push(std::get(maybe_type_id)); - } else if (auto type_id = std::get(maybe_type_id); - type_id.has_value()) { - Push(type_id); + if (sem_ir_->types().IsFacetType(sem_ir_->insts().Get(inst_id).type_id())) { + Push(TypeValue{.inst_id = inst_id}); } else if (sem_ir_->constant_values().Get(inst_id).is_symbolic()) { Push(SymbolicNonTypeValue{.inst_id = inst_id}); } else { @@ -329,6 +311,10 @@ auto TypeIterator::PushInstId(InstId inst_id) -> void { } } +auto TypeIterator::PushTypeId(TypeId type_id) -> void { + Push(TypeValue{.inst_id = sem_ir_->types().GetTypeInstId(type_id)}); +} + // Push the next step into the work queue. auto TypeIterator::Push(WorkItem item) -> void { work_list_.push_back(item); } diff --git a/toolchain/sem_ir/type_iterator.h b/toolchain/sem_ir/type_iterator.h index e966d94d396c..8868c27f5efb 100644 --- a/toolchain/sem_ir/type_iterator.h +++ b/toolchain/sem_ir/type_iterator.h @@ -65,14 +65,9 @@ class TypeIterator { private: // A work item to mark the end of an aggregate type's scope. struct EndType {}; - // A work item to mark a symbolic type. - struct SymbolicType { - EntityNameId entity_name_id; - InstId facet; - }; - // A work item to mark a FacetValue instruction. - struct FacetValueItem { - InstId facet_value_inst_id; + // A work item to mark a concrete or symbolic type. + struct TypeValue { + InstId inst_id; }; // A work item to mark a concrete non-type value. struct ConcreteNonTypeValue { @@ -88,21 +83,12 @@ class TypeIterator { }; using WorkItem = - std::variant; + std::variant; - // Processes `next` when it's a `TypeId`. - auto ProcessTypeId(TypeId type_id) -> std::optional; - - // Get the TypeId for an instruction that is not a facet value, otherwise - // return SymbolicType to indicate the instruction is a symbolic facet value. - // - // If the instruction is not a type value, the return is TypeId::None. - // - // We reuse the `SymbolicType` work item here to give a nice return type. - auto TryGetInstIdAsTypeId(InstId inst_id) const - -> std::variant; + // Processes `next` when it's a type value (concrete or symbolic). + auto ProcessType(InstId inst_id) -> std::optional; // Get the instructions in the specific's instruction block as an ArrayRef. auto GetSpecificArgs(SpecificId specific_id) const -> llvm::ArrayRef; @@ -110,9 +96,10 @@ class TypeIterator { // Push all arguments from the array into the work queue. auto PushArgs(llvm::ArrayRef args) -> void; - // Push an instruction's type value into the work queue, or a marker if the - // instruction has a symbolic value. + // Push an instruction into the work queue. auto PushInstId(InstId inst_id) -> void; + // Push a type id into the work queue. + auto PushTypeId(TypeId type_id) -> void; // Push the next step into the work queue. auto Push(WorkItem item) -> void; @@ -177,7 +164,7 @@ class TypeIterator::Step { // Individual result values, which appear on their own or inside some scope // that begin with `StartWithEnd`. - // A type value. + // A concrete type value. struct ConcreteType { TypeId type_id; }; @@ -189,16 +176,25 @@ class TypeIterator::Step { // The facet, whose type is either a FacetType or the TypeType singleton. InstId facet; }; - // A FacetValue, representing the conversion of a type to a facet type, which - // may be symbolic or concrete depending on the type inside. The iterator will - // iterate into the FacetValue, so it's reasonable to ignore this step, unless - // the consumer wants to specifically collect FacetValues. - struct FacetValue { - // The FacetValue instruction. - InstId facet_value_inst_id; - }; // A symbolic template type value. struct TemplateType {}; + + // A wrapper instruction around a type, which may be concrete, symbolic, or + // template depending on the type inside. The iterator will iterate into the + // wrapper instruction, so it's reasonable to ignore this step, unless the + // consumer wants to specifically collect them. + struct TypeWrapper { + enum Kind { + // A FacetValue, representing the conversion of a type to a facet type. + FacetValue, + // An access of an associated constant in a facet. + ImplWitnessAccess, + }; + Kind kind; + // The wrapper instruction. + InstId inst_id; + }; + // A concrete non-type value, which can be found as a generic parameter for a // type. struct ConcreteValue { @@ -228,7 +224,7 @@ class TypeIterator::Step { // Each step is one of these. using Any = std::variant< - ConcreteType, SymbolicType, FacetValue, TemplateType, ConcreteValue, + ConcreteType, SymbolicType, TemplateType, TypeWrapper, ConcreteValue, SymbolicValue, StructFieldName, ClassStartOnly, StructStartOnly, TupleStartOnly, InterfaceStartOnly, NamedConstraintStartOnly, ClassStart, StructStart, TupleStart, InterfaceStart, NamedConstraintStart, IntStart,