carbon-language/carbon-lang
1
0
Fork 0
mirror of https://github.com/carbon-language/carbon-lang.git synced 2026-09-24 19:40:10 +01:00
Code Issues Packages Projects Releases 10 Wiki Activity

During identify replace .Self only in the initial facet type (#7436)

Browse Source 
When we find a named constraint during identity, we recurse into it. The
specific args of the named constraint may contain references to `.Self`
which can then make `.Self` appear inside the named constraint, which
was making us replace `.Self` at multiple levels and incorrectly. The
first specific argument replaces `Self` in the named constraint, and we
pass in the self-type of the identify operation. This may contain
`.Self` and we should _not_ be replacing the `.Self` references with the
self-type that they are contained within. This led to infinite cycles.

In the meantime, we have made the toolchain reject any ambiguous `.Self`
from being constructed. So we know there is only one value of `.Self`
around in a facet type.

So now we replace `.Self` only in the top level facet type during
identity. That means replacing `.Self` in the specifics of the named
constraints that we recurse into. But we do _not_ replace `.Self`
anymore inside those named constraints. This resolves the infinite loop.

At the same time, when we are identifying an `impls` constraint from
earlier in the same facet type, like `C impls Z(.Self)` we are
identifying with a self-type of `C`. We want the output to use `C` as
the self-type since we should get back an identified facet type that
says `C impls Z(.Self)`. But we do _not_ want to replace the `.Self`
there since we're inside a facet type and the `.Self` does not refer to
`C`. So we parameterize `TryToIdentifyFacetType` to not replace `.Self`
when identifying an `impls` constraint from the `where_stack()`. This
resolves a large number of `fail_todo_` tests.
This commit is contained in:
Dana Jansens
2026-06-30 23:33:30 +00:00
committed by GitHub
parent 8928268a95
commit e7771c2f6d
4 changed files with 101 additions and 100 deletions
Download Patch File Download Diff File Expand all files Collapse all files
toolchain/check/impl_lookup.cpp
+16 -16
View File
@@ -372,16 +372,13 @@ static auto CollectFacetWitnessSources(
if (type_id != SemIR::TypeType::TypeId) {
auto facet_type = context.types().GetAs<SemIR::FacetType>(type_id);
// When we identify the facet type within a facet in the query, we also
// replace `.Self` with the `facet_const_id`, which has the same type as
// the facet type we're identifying. If that `.Self` replacement is in a
// `LookupImplWitness` instruction, it will evaluate and look for a final
// impl. When we find a generic `final impl` and try to deduce its
// parameters, we may end up trying to convert the `facet_const_id` which
// does an impl lookup and comes back here. If we replace `.Self` again,
// we just cycle indefinitely identifying and substituting
// `facet_const_id` into its type. We break that cycle here by preventing
// `final impl` lookups while replacing `.Self`.
// Identifying the facet type of `facet_const_id` causes `.Self` to be
// replaced with `facet_const_id`. The resulting `LookupImplWitness`
// evaluation searches for a `final impl`. The toolchain needs to deduce
// the result's parameters when a generic `final impl` is found. Deduction
// may convert the `facet_const_id`, which returns here and then we loop
// forever. We break that cycle here by preventing `final impl` lookups
// while replacing `.Self`.
//
// TODO: This prevents some legitimate code from working, as we can't find
// some witnesses that involve concrete associated constants from a
@@ -390,11 +387,13 @@ static auto CollectFacetWitnessSources(
// facet type, remove that constraint from the type of `facet_const_id`.
// Then each cycle back through to `facet_const_id` will have a smaller
// type until it runs out of constraints.
context.impl_lookup_no_symbolic_final_lookups()++;
auto identified_id =
TryToIdentifyFacetType(context, loc_id, facet_const_id, facet_type,
allow_partially_identified);
context.impl_lookup_no_symbolic_final_lookups()--;
auto& no_symbolic_final_lookups =
context.impl_lookup_no_symbolic_final_lookups();
++no_symbolic_final_lookups;
auto identified_id = TryToIdentifyFacetType(
context, loc_id, facet_const_id, facet_type,
allow_partially_identified, /*subst_period_self=*/true);
--no_symbolic_final_lookups;
if (identified_id.has_value()) {
witnesses.push_back({.facet_const_id = facet_const_id,
@@ -475,7 +474,8 @@ static auto CollectFacetWitnessSources(
facet_type_const_id);
auto identified_id = TryToIdentifyFacetType(
context, loc_id, canon_self_const_id, facet_type,
/*allow_partially_identified=*/true);
/*allow_partially_identified=*/true,
/*subst_period_self=*/false);
if (identified_id.has_value()) {
witnesses.push_back({.facet_const_id = canon_self_const_id,
.identified_facet_type_id = identified_id});
toolchain/check/testdata/facet/early_impls.carbon
Vendored
+34 -56
View File
@@ -58,7 +58,7 @@ class C(T:! type);
fn F(_:! Z where C(.Self) impls Y and .Z1 = C(.Self).(Y.Y1)) {}
// --- fail_todo_early_class_impls_generic_interface.carbon
// --- early_class_impls_generic_interface.carbon
library "[[@TEST_NAME]]";
interface Z {
@@ -70,10 +70,6 @@ class C;
// Needs to identify `C as Y(.Self)` without replacing this `.Self` with `C`,
// since it refers to `T`.
// CHECK:STDERR: fail_todo_early_class_impls_generic_interface.carbon:[[@LINE+4]]:53: error: cannot convert type `C` into type implementing `Y(.Self)` [ConversionFailureTypeToFacet]
// CHECK:STDERR: fn F(unused T:! Z where C impls Y(.Self) and .Z1 = (C as Y(.Self))) {}
// CHECK:STDERR: ^~~~~~~~~~~~~
// CHECK:STDERR:
fn F(unused T:! Z where C impls Y(.Self) and .Z1 = (C as Y(.Self))) {}
class D(V:! type);
@@ -82,16 +78,9 @@ class D(V:! type);
// `.Self` to `D` since it refers to `T`. But we _do_ need to replace the
// `.Self` in the type of `U`, since it refers to `U` which is being replaced by
// `D(.Self)` in this specific.
// CHECK:STDERR: fail_todo_early_class_impls_generic_interface.carbon:[[@LINE+7]]:71: error: cannot convert type `D(.Self)` into type implementing `Y(.Self)` [ConversionFailureTypeToFacet]
// CHECK:STDERR: fn G(unused T:! Z where D(.Self) impls Y(D(.Self)) and D(.Self) impls X(D(.Self))) {}
// CHECK:STDERR: ^~~~~~~~~~~
// CHECK:STDERR: fail_todo_early_class_impls_generic_interface.carbon:[[@LINE-20]]:1: note: while deducing parameters of generic declared here [DeductionGenericHere]
// CHECK:STDERR: interface X(U:! Y(.Self)) {}
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~
// CHECK:STDERR:
fn G(unused T:! Z where D(.Self) impls Y(D(.Self)) and D(.Self) impls X(D(.Self))) {}
// --- fail_todo_early_class_impls_generic_interface_with_member_designator.carbon
// --- early_class_impls_generic_interface_with_member_designator.carbon
library "[[@TEST_NAME]]";
interface Z {
@@ -103,13 +92,9 @@ interface Y(T:! type) {
}
class C;
// CHECK:STDERR: fail_todo_early_class_impls_generic_interface_with_member_designator.carbon:[[@LINE+4]]:43: error: cannot convert type `C` into type implementing `Y(.(Z.Z2))` [ConversionFailureTypeToFacet]
// CHECK:STDERR: fn F(_:! Z where C impls Y(.Z2) and .Z1 = C.(Y(.Z2).Y1)) {}
// CHECK:STDERR: ^~~~~~~~~~~~~
// CHECK:STDERR:
fn F(_:! Z where C impls Y(.Z2) and .Z1 = C.(Y(.Z2).Y1)) {}
// --- fail_todo_early_tuple_impls_generic_interface.carbon
// --- early_tuple_impls_generic_interface.carbon
library "[[@TEST_NAME]]";
interface Z {
@@ -123,33 +108,13 @@ interface X {
}
class C;
// CHECK:STDERR: fail_todo_early_tuple_impls_generic_interface.carbon:[[@LINE+4]]:46: error: cannot convert type `()` into type implementing `Y(.Self)` [ConversionFailureTypeToFacet]
// CHECK:STDERR: fn F(_:! Z where () impls Y(.Self) and .Z1 = ().(Y(.Self).Y1)) {}
// CHECK:STDERR: ^~~~~~~~~~~~~~~~
// CHECK:STDERR:
fn F(_:! Z where () impls Y(.Self) and .Z1 = ().(Y(.Self).Y1)) {}
// CHECK:STDERR: fail_todo_early_tuple_impls_generic_interface.carbon:[[@LINE+4]]:49: error: cannot convert type `(C,)` into type implementing `Y(.Self)` [ConversionFailureTypeToFacet]
// CHECK:STDERR: fn G(_:! Z where (C, ) impls Y(.Self) and .Z1 = (C, ).(Y(.Self).Y1)) {}
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~
// CHECK:STDERR:
fn G(_:! Z where (C, ) impls Y(.Self) and .Z1 = (C, ).(Y(.Self).Y1)) {}
// TODO: Can merge with the next test once they both pass.
// --- early_tuple_impls.carbon
library "[[@TEST_NAME]]";
interface Z {
let Z1:! type;
}
interface X {
let X1:! type;
}
fn H(_:! Z where (.Self, ) impls X and .Z1 = (.Self, ).(X.X1)) {}
// --- fail_todo_early_struct_impls.carbon
// --- early_struct_impls.carbon
library "[[@TEST_NAME]]";
interface Z {
@@ -163,13 +128,9 @@ interface X {
}
class C;
// CHECK:STDERR: fail_todo_early_struct_impls.carbon:[[@LINE+4]]:46: error: cannot convert type `{}` into type implementing `Y(.Self)` [ConversionFailureTypeToFacet]
// CHECK:STDERR: fn F(_:! Z where {} impls Y(.Self) and .Z1 = {}.(Y(.Self).Y1)) {}
// CHECK:STDERR: ^~~~~~~~~~~~~~~~
// CHECK:STDERR:
fn F(_:! Z where {} impls Y(.Self) and .Z1 = {}.(Y(.Self).Y1)) {}
// --- fail_todo_early_concrete_impls.carbon
// --- early_concrete_impls.carbon
library "[[@TEST_NAME]]";
interface Z {
@@ -183,10 +144,6 @@ interface Y(T:! type) {
fn MakeIntLiteral() -> type = "int_literal.make_type";
alias IntLiteral = MakeIntLiteral();
// CHECK:STDERR: fail_todo_early_concrete_impls.carbon:[[@LINE+4]]:54: error: cannot convert type `IntLiteral` into type implementing `Y(.Self)` [ConversionFailureTypeToFacet]
// CHECK:STDERR: fn F(_:! Z where IntLiteral impls Y(.Self) and .Z1 = IntLiteral.(Y(.Self).Y1)) {}
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~
// CHECK:STDERR:
fn F(_:! Z where IntLiteral impls Y(.Self) and .Z1 = IntLiteral.(Y(.Self).Y1)) {}
// --- early_impl_named_constraint.carbon
@@ -244,7 +201,7 @@ constraint N(V:! type) {
// CHECK:STDERR:
fn F(_:! Z(.Self) where C impls N(.Z1) and .Z2 = (.Z1 as Y(.Self))) {}
// --- fail_todo_early_type_impls_nested_self_impls.carbon
// --- early_type_impls_nested_self_impls.carbon
library "[[@TEST_NAME]]";
interface Z {
@@ -265,11 +222,32 @@ constraint NY {
// A lookup of `C(V).(Y.Y1) as X` requires us to see that the `.Y1 impls X`
// constraint is visible through the `C(.Self) impls NY` constraint and that
// `C(.Self)` is used as the implied `.Self` in `.Y1 impls X`.
// CHECK:STDERR: fail_todo_early_type_impls_nested_self_impls.carbon:[[@LINE+7]]:53: error: cannot convert type `C(.Self).(Y.Y1)` into type implementing `X` [ConversionFailureTypeToFacet]
// CHECK:STDERR: fn F(unused V:! Z where C(.Self) impls NY and .Z1 = D(C(.Self).(Y.Y1))) {}
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~
// CHECK:STDERR: fail_todo_early_type_impls_nested_self_impls.carbon:[[@LINE-12]]:9: note: initializing generic parameter `T` declared here [InitializingGenericParam]
// CHECK:STDERR: class D(T:! X);
// CHECK:STDERR: ^~~~~
// CHECK:STDERR:
fn F(unused V:! Z where C(.Self) impls NY and .Z1 = D(C(.Self).(Y.Y1))) {}
// --- period_self_impls_named_constraint_used_in_later_constraint.carbon
library "[[@TEST_NAME]]";
interface X {}
interface Y {
let Y1:! type;
}
constraint ConstraintForY {
require impls Y where .Y1 impls X;
}
interface Z {
let Z1:! type;
}
private constraint ConstraintForZ {
extend require impls Z where .Z1 impls ConstraintForY;
}
// If we replace `.Self` inside the named constraint `ConstraintForY`, we end up
// replacing `.Self` in the `.Z1` from the specific args for the named
// constraint, which replaces `Self` in the named constraint. Doing this is
// incorrect and was leading to an infinite loop of `.Self` substitution.
fn F(_:! ConstraintForZ where
.Z1 impls ConstraintForY and
.Z1.(Y.Y1) == ()) {}
toolchain/check/type_completion.cpp
+49 -27
View File
@@ -965,7 +965,8 @@ static auto GetSelfFacetValue(Context& context, SemIR::ConstantId self_const_id)
static auto IdentifyFacetType(Context& context, SemIR::LocId loc_id,
SemIR::ConstantId initial_self_const_id,
const SemIR::FacetType& facet_type,
bool allow_partially_identified, bool diagnose)
bool allow_partially_identified,
bool initial_subst_period_self, bool diagnose)
-> SemIR::IdentifiedFacetTypeId {
// While partially identified facet types end up in the store of
// IdentifiedFacetTypes, we don't try to construct a key to look for them
@@ -986,18 +987,16 @@ static auto IdentifyFacetType(Context& context, SemIR::LocId loc_id,
// Whether the impling of facet type should be considered as extending in
// the resulting IdentifiedFacetType.
bool extend;
// Whether we should replace `.Self` in the self type. This is false for the
// top-level self, since we'd be replacing parts of it with itself, which is
// cyclical. It becomes true when recursing into a `T impls ...` constraint
// where the self type is now something else.
bool subst_self;
// Whether we should replace `.Self` in the constraint.
bool subst_period_self;
SemIR::ConstantId self;
SemIR::FacetTypeId facet_type;
};
// Work queue.
llvm::SmallVector<SelfImplsFacetType> work = {
{true, false, initial_self_const_id, facet_type.facet_type_id}};
{true, initial_subst_period_self, initial_self_const_id,
facet_type.facet_type_id}};
// Outputs for the IdentifiedFacetType.
bool partially_identified = false;
@@ -1010,30 +1009,34 @@ static auto IdentifyFacetType(Context& context, SemIR::LocId loc_id,
while (!work.empty()) {
SelfImplsFacetType next_impls = work.pop_back_val();
bool facet_type_extends = next_impls.extend;
auto subst_period_self_in_self = next_impls.subst_self;
auto subst_period_self = next_impls.subst_period_self;
auto self_const_id = GetCanonicalFacetOrTypeValue(context, next_impls.self);
const auto& facet_type_info =
context.facet_types().Get(next_impls.facet_type);
auto self_and_interface = [&](SemIR::SpecificInterface impls_interface)
-> SemIR::IdentifiedFacetType::RequiredImpl {
auto self = subst_period_self_in_self
? SubstPeriodSelf(context, loc_id, self_const_id,
period_self_replacement_id)
: self_const_id;
auto interface = SubstPeriodSelf(context, loc_id, impls_interface,
period_self_replacement_id);
auto self = self_const_id;
auto interface = subst_period_self
? SubstPeriodSelf(context, loc_id, impls_interface,
period_self_replacement_id)
: impls_interface;
return {self, interface};
};
auto type_and_interface =
[&](SemIR::FacetTypeInfo::TypeImplsInterface impls)
-> SemIR::IdentifiedFacetType::RequiredImpl {
auto self = SubstPeriodSelf(
context, loc_id, context.constant_values().Get(impls.self_type),
period_self_replacement_id);
auto self =
subst_period_self
? SubstPeriodSelf(context, loc_id,
context.constant_values().Get(impls.self_type),
period_self_replacement_id)
: context.constant_values().Get(impls.self_type);
auto interface =
SubstPeriodSelf(context, loc_id, impls.specific_interface,
period_self_replacement_id);
subst_period_self
? SubstPeriodSelf(context, loc_id, impls.specific_interface,
period_self_replacement_id)
: impls.specific_interface;
return {self, interface};
};
@@ -1065,6 +1068,11 @@ static auto IdentifyFacetType(Context& context, SemIR::LocId loc_id,
auto self_facet = GetSelfFacetValue(context, self_const_id);
for (auto extends : facet_type_info.extend_named_constraints) {
if (subst_period_self) {
extends = SubstPeriodSelf(context, loc_id, extends,
period_self_replacement_id);
}
const auto& constraint =
context.named_constraints().Get(extends.named_constraint_id);
@@ -1118,12 +1126,16 @@ static auto IdentifyFacetType(Context& context, SemIR::LocId loc_id,
.GetInstAs<SemIR::FacetType>(require_facet_type)
.facet_type_id;
bool extend = facet_type_extends && require.extend_self;
work.push_back(
{extend, subst_period_self_in_self, require_self, facet_type_id});
work.push_back({extend, false, require_self, facet_type_id});
}
}
for (auto impls : facet_type_info.self_impls_named_constraints) {
if (subst_period_self) {
impls =
SubstPeriodSelf(context, loc_id, impls, period_self_replacement_id);
}
const auto& constraint =
context.named_constraints().Get(impls.named_constraint_id);
@@ -1175,14 +1187,21 @@ static auto IdentifyFacetType(Context& context, SemIR::LocId loc_id,
context.constant_values()
.GetInstAs<SemIR::FacetType>(require_facet_type)
.facet_type_id;
work.push_back(
{false, subst_period_self_in_self, require_self, facet_type_id});
work.push_back({false, false, require_self, facet_type_id});
}
}
for (const auto& type_impls :
facet_type_info.type_impls_named_constraints) {
auto [self_type_inst_id, impls] = type_impls;
if (subst_period_self) {
self_type_inst_id = context.constant_values().GetInstId(SubstPeriodSelf(
context, loc_id, context.constant_values().Get(self_type_inst_id),
period_self_replacement_id));
impls =
SubstPeriodSelf(context, loc_id, impls, period_self_replacement_id);
}
const auto& constraint =
context.named_constraints().Get(impls.named_constraint_id);
@@ -1237,7 +1256,7 @@ static auto IdentifyFacetType(Context& context, SemIR::LocId loc_id,
context.constant_values()
.GetInstAs<SemIR::FacetType>(require_facet_type)
.facet_type_id;
work.push_back({false, true, require_self, facet_type_id});
work.push_back({false, false, require_self, facet_type_id});
}
}
}
@@ -1250,10 +1269,12 @@ static auto IdentifyFacetType(Context& context, SemIR::LocId loc_id,
auto TryToIdentifyFacetType(Context& context, SemIR::LocId loc_id,
SemIR::ConstantId self_const_id,
const SemIR::FacetType& facet_type,
bool allow_partially_identified)
bool allow_partially_identified,
bool subst_period_self)
-> SemIR::IdentifiedFacetTypeId {
return IdentifyFacetType(context, loc_id, self_const_id, facet_type,
allow_partially_identified, /*diagnose=*/false);
allow_partially_identified, subst_period_self,
/*diagnose=*/false);
}
auto RequireIdentifiedFacetType(Context& context, SemIR::LocId loc_id,
@@ -1265,7 +1286,8 @@ auto RequireIdentifiedFacetType(Context& context, SemIR::LocId loc_id,
Diagnostics::ContextScope scope(&context.emitter(), diagnostic_context);
return IdentifyFacetType(context, loc_id, self_const_id, facet_type,
/*allow_partially_identified=*/false, diagnose);
/*allow_partially_identified=*/false,
/*initial_subst_period_self=*/true, diagnose);
}
} // namespace Carbon::Check
toolchain/check/type_completion.h
+2 -1
View File
@@ -78,7 +78,8 @@ auto RequireConcreteType(Context& context, SemIR::TypeId type_id,
auto TryToIdentifyFacetType(Context& context, SemIR::LocId loc_id,
SemIR::ConstantId self_const_id,
const SemIR::FacetType& facet_type,
bool allow_partially_identified)
bool allow_partially_identified,
bool subst_period_self)
-> SemIR::IdentifiedFacetTypeId;
// Requires the named constraints in the facet type to be complete, so that the