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,