From 0848cf941da92292420ec8972ea6de096ccaf9ec Mon Sep 17 00:00:00 2001 From: Dana Jansens Date: Thu, 16 Jul 2026 15:00:06 -0400 Subject: [PATCH] Look outside constant values for designators in `where` constraints (#7367) The constant value may lose the designator during eval, such as an `ImplWitnessAccess` that resolves to some concrete type. Look in the non-canonical instructions instead. --- toolchain/check/handle_alias.cpp | 12 +- toolchain/check/handle_where.cpp | 131 ++++++------ toolchain/check/name_component.cpp | 7 +- toolchain/check/name_component.h | 6 +- .../facet/nested_facet_types_from_eval.carbon | 7 +- .../testdata/where_expr/constraints.carbon | 7 +- .../testdata/where_expr/designator.carbon | 199 +++++++++++++++++- 7 files changed, 289 insertions(+), 80 deletions(-) diff --git a/toolchain/check/handle_alias.cpp b/toolchain/check/handle_alias.cpp index 4338345d4e02..c3dcf083ed66 100644 --- a/toolchain/check/handle_alias.cpp +++ b/toolchain/check/handle_alias.cpp @@ -35,8 +35,16 @@ auto HandleParseNode(Context& /*context*/, auto HandleParseNode(Context& context, Parse::AliasId /*node_id*/) -> bool { auto [expr_node, expr_id] = context.node_stack().PopExprWithNodeId(); - auto name_context = context.decl_name_stack().FinishName( - PopNameComponentWithoutParams(context, Lex::TokenKind::Alias)); + bool diagnosed_params = false; + auto name_context = + context.decl_name_stack().FinishName(PopNameComponentWithoutParams( + context, Lex::TokenKind::Alias, &diagnosed_params)); + if (diagnosed_params) { + // If the alias had generic parameters, they could be SymbolicBindings which + // then appear in the `expr_id`. Using those elsewhere is then invalid and + // creates invalid states in the toolchain. + expr_id = SemIR::ErrorInst::InstId; + } DiscardGenericDecl(context); diff --git a/toolchain/check/handle_where.cpp b/toolchain/check/handle_where.cpp index f28fa87afdeb..705cdc34d7d4 100644 --- a/toolchain/check/handle_where.cpp +++ b/toolchain/check/handle_where.cpp @@ -186,9 +186,8 @@ auto HandleParseNode(Context& context, Parse::WhereOperandId node_id) -> bool { } // Returns whether a designator (`.Self` or `.MemberName`) is present in -// `inst_id`. -static auto FindDesignator(Context& context, SemIR::ConstantId const_id) - -> bool { +// `inst_id` in a way that will constrain the current `.Self`. +static auto FindDesignator(Context& context, SemIR::InstId inst_id) -> bool { class SubstFindDesignator : public SubstInstCallbacks { public: explicit SubstFindDesignator(Context* context, bool* found) @@ -206,13 +205,25 @@ static auto FindDesignator(Context& context, SemIR::ConstantId const_id) } // TypeType has type TypeType, avoid recursing on its type. - if (context().insts().Is(inst_id)) { + if (inst_id == SemIR::TypeType::TypeInstId) { return FullySubstituted; } + // Arguments to a call do not count, since a call with `.Self` in it will + // not be evaluated inside the facet type. + if (context().insts().Is(inst_id)) { + return FullySubstituted; + } + + // TODO: When we support parameterized aliases, if an argument has + // `.Self`, we will need to evaluate the alias here and look for `.Self` + // in the constant value. + // `.MemberName` is represented as an ImplWitnessAccess through `.Self` so // we only need to look for `.Self` here. - if (IsPeriodSelf(context(), inst_id)) { + // + // Subst will recurse into operands, so we don't want to canonicalize. + if (IsPeriodSelf(context(), inst_id, /*canonicalize=*/false)) { *found_ = true; return FullySubstituted; } @@ -228,15 +239,9 @@ static auto FindDesignator(Context& context, SemIR::ConstantId const_id) bool* found_; }; - // A facet type may contain designators but they do not constrain this where - // clause's type. - if (context.constant_values().InstIs(const_id)) { - return false; - } - bool found = false; SubstFindDesignator callbacks(&context, &found); - SubstInst(context, context.constant_values().GetInstId(const_id), callbacks); + SubstInst(context, inst_id, callbacks); return found; } @@ -295,25 +300,22 @@ auto HandleParseNode(Context& context, Parse::RequirementEqualId node_id) auto HandleParseNode(Context& context, Parse::RequirementEqualEqualId node_id) -> bool { - auto rhs = context.node_stack().PopExpr(); - auto lhs = context.node_stack().PopExpr(); + auto rhs_id = context.node_stack().PopExpr(); + auto lhs_id = context.node_stack().PopExpr(); // TODO: Type check lhs and rhs are comparable. - auto const_lhs = context.constant_values().Get(lhs); - auto const_rhs = context.constant_values().Get(rhs); - if (!FindDesignator(context, const_lhs) && - !FindDesignator(context, const_rhs)) { - if (const_lhs != SemIR::ErrorInst::ConstantId && - const_rhs != SemIR::ErrorInst::ConstantId) { + if (!FindDesignator(context, lhs_id) && !FindDesignator(context, rhs_id)) { + if (context.constant_values().Get(lhs_id) != SemIR::ErrorInst::ConstantId && + context.constant_values().Get(rhs_id) != SemIR::ErrorInst::ConstantId) { DiagnoseMissingDesignator(context, node_id); } - lhs = rhs = SemIR::ErrorInst::InstId; + lhs_id = rhs_id = SemIR::ErrorInst::InstId; } // Build up the list of arguments for the `WhereExpr` inst. context.args_type_info_stack().AddInstId( - AddInst(context, node_id, - {.lhs_id = lhs, .rhs_id = rhs})); + AddInst( + context, node_id, {.lhs_id = lhs_id, .rhs_id = rhs_id})); return true; } @@ -343,52 +345,59 @@ static auto IsPeriodSelfAccess(Context& context, SemIR::InstId inst_id) } } +static auto FindDesignatorInSpecific(Context& context, + SemIR::SpecificId specific_id) -> bool { + for (auto inst_id : context.inst_blocks().Get( + context.specifics().GetArgsOrEmpty(specific_id))) { + if (FindDesignator(context, inst_id)) { + return true; + } + } + return false; +} + +static auto FindDesignatorInEveryExtendConstraint(Context& context, + SemIR::FacetType facet_type) + -> bool { + const auto& info = context.facet_types().Get(facet_type.facet_type_id); + + for (const auto& extend : info.extend_constraints) { + if (!FindDesignatorInSpecific(context, extend.specific_id)) { + return false; + } + } + for (const auto& extend : info.extend_named_constraints) { + if (!FindDesignatorInSpecific(context, extend.specific_id)) { + return false; + } + } + return !info.extend_constraints.empty() || + !info.extend_named_constraints.empty(); +} + auto HandleParseNode(Context& context, Parse::RequirementImplsId node_id) -> bool { auto [rhs_node, rhs_id] = context.node_stack().PopExprWithNodeId(); auto [lhs_node, lhs_id] = context.node_stack().PopExprWithNodeId(); - auto const_lhs = context.constant_values().Get(lhs_id); - auto const_rhs = context.constant_values().Get(rhs_id); - if (!FindDesignator(context, const_lhs)) { - // The RHS of an `impls` may be another `where`. We require a designator to - // be present in each constraint created from the LHS of that `where`, which - // equates to requiring a designator in each extend constraint of the facet - // type. - // - // If a designator is part of the LHS of the `impls` or the LHS of the inner - // `where`, then that implies all constraints nested within the `where` - // clause will constrain the top level type in some way. + if (!FindDesignator(context, lhs_id)) { + bool found_designator = false; + auto const_rhs_id = context.constant_values().Get(rhs_id); if (auto facet_type = context.constant_values().TryGetInstAs( - const_rhs)) { - const auto& info = context.facet_types().Get(facet_type->facet_type_id); - for (auto extend : llvm::concat( - llvm::map_range( - info.extend_constraints, - [](SemIR::SpecificInterface i) { return i.specific_id; }), - llvm::map_range(info.extend_named_constraints, - [](SemIR::SpecificNamedConstraint c) { - return c.specific_id; - }))) { - bool found_designator = false; - for (auto inst_id : context.inst_blocks().Get( - context.specifics().GetArgsOrEmpty(extend))) { - if (FindDesignator(context, context.constant_values().Get(inst_id))) { - found_designator = true; - break; - } - } - if (!found_designator) { - if (const_lhs != SemIR::ErrorInst::ConstantId && - const_rhs != SemIR::ErrorInst::ConstantId) { - DiagnoseMissingDesignator(context, node_id); - } - lhs_id = rhs_id = SemIR::ErrorInst::InstId; - const_lhs = const_rhs = SemIR::ErrorInst::ConstantId; - break; - } + const_rhs_id)) { + found_designator = + FindDesignatorInEveryExtendConstraint(context, *facet_type); + } + if (!found_designator) { + auto const_lhs_id = context.constant_values().Get(lhs_id); + if (const_lhs_id != SemIR::ErrorInst::ConstantId && + const_rhs_id != SemIR::ErrorInst::ConstantId) { + // TODO: Can we diagnose the specific constraint that was missing the + // `.Self`? + DiagnoseMissingDesignator(context, node_id); } + lhs_id = rhs_id = SemIR::ErrorInst::InstId; } } diff --git a/toolchain/check/name_component.cpp b/toolchain/check/name_component.cpp index e2b62c653f16..2ec42b60cc60 100644 --- a/toolchain/check/name_component.cpp +++ b/toolchain/check/name_component.cpp @@ -81,8 +81,8 @@ auto PopNameComponent(Context& context, SemIR::InstId return_pattern_id) // Pop the name of a declaration from the node stack, and diagnose if it has // parameters. -auto PopNameComponentWithoutParams(Context& context, Lex::TokenKind introducer) - -> NameComponent { +auto PopNameComponentWithoutParams(Context& context, Lex::TokenKind introducer, + bool* diagnosed_params) -> NameComponent { NameComponent name = PopNameComponent(context); if (name.call_params_id.has_value()) { CARBON_DIAGNOSTIC(UnexpectedDeclNameParams, Error, @@ -95,6 +95,9 @@ auto PopNameComponentWithoutParams(Context& context, Lex::TokenKind introducer) UnexpectedDeclNameParams, introducer); name.call_params_id = SemIR::InstBlockId::None; + if (diagnosed_params) { + *diagnosed_params = true; + } } return name; } diff --git a/toolchain/check/name_component.h b/toolchain/check/name_component.h index 0430e734784d..c822e957ffdb 100644 --- a/toolchain/check/name_component.h +++ b/toolchain/check/name_component.h @@ -53,8 +53,10 @@ auto PopNameComponent(Context& context, -> NameComponent; // Equivalent to PopNameComponent, but also diagnoses if the name component has -// parameters. -auto PopNameComponentWithoutParams(Context& context, Lex::TokenKind introducer) +// parameters. Sets `*diagnosed_params` to true when diagnosing parameters if +// it's not null. +auto PopNameComponentWithoutParams(Context& context, Lex::TokenKind introducer, + bool* diagnosed_params = nullptr) -> NameComponent; } // namespace Carbon::Check diff --git a/toolchain/check/testdata/facet/nested_facet_types_from_eval.carbon b/toolchain/check/testdata/facet/nested_facet_types_from_eval.carbon index c23bb8a80417..d99fe4e4a005 100644 --- a/toolchain/check/testdata/facet/nested_facet_types_from_eval.carbon +++ b/toolchain/check/testdata/facet/nested_facet_types_from_eval.carbon @@ -21,12 +21,9 @@ alias A = I where .Self == C; // The use of `A` introduces a `where` expression inside the `where` written // here, which is an error. // -// CHECK:STDERR: fail_where_nested_inside_where_through_alias_impls_lhs.carbon:[[@LINE+7]]:28: error: found `where` expression nested on the right-hand side of `where` [NestedWhereInsideWhere] +// CHECK:STDERR: fail_where_nested_inside_where_through_alias_impls_lhs.carbon:[[@LINE+4]]:28: error: constraint in `where` clause without a designator; expected `.Self` or a member access like `.M` [WhereWithoutDesignator] // CHECK:STDERR: fn F(generic _: type where C(A) impls type) {} -// CHECK:STDERR: ^~~~ -// CHECK:STDERR: fail_where_nested_inside_where_through_alias_impls_lhs.carbon:[[@LINE+4]]:17: note: on right-hand side of `where` here [NestedWhereInsideWhereOuterNote] -// CHECK:STDERR: fn F(generic _: type where C(A) impls type) {} -// CHECK:STDERR: ^~~~~~~~~~ +// CHECK:STDERR: ^~~~~~~~~~~~~~~ // CHECK:STDERR: fn F(generic _: type where C(A) impls type) {} diff --git a/toolchain/check/testdata/where_expr/constraints.carbon b/toolchain/check/testdata/where_expr/constraints.carbon index b6746e13ba3d..b84160c411ee 100644 --- a/toolchain/check/testdata/where_expr/constraints.carbon +++ b/toolchain/check/testdata/where_expr/constraints.carbon @@ -81,12 +81,9 @@ fn AssociatedTypeImpls(generic W: K where .Associated impls M); library "[[@TEST_NAME]]"; -// CHECK:STDERR: fail_left_of_impls_non_type.carbon:[[@LINE+7]]:39: error: cannot implicitly convert non-type value of type `Core.IntLiteral` to `type` [ConversionFailureNonTypeToFacet] +// CHECK:STDERR: fail_left_of_impls_non_type.carbon:[[@LINE+4]]:39: error: constraint in `where` clause without a designator; expected `.Self` or a member access like `.M` [WhereWithoutDesignator] // CHECK:STDERR: fn NonTypeImpls(generic U: type where 7 impls type); -// CHECK:STDERR: ^ -// CHECK:STDERR: fail_left_of_impls_non_type.carbon:[[@LINE+4]]:39: note: type `Core.IntLiteral` does not implement interface `Core.ImplicitAs(type)` [MissingImplInMemberAccessInContext] -// CHECK:STDERR: fn NonTypeImpls(generic U: type where 7 impls type); -// CHECK:STDERR: ^ +// CHECK:STDERR: ^~~~~~~~~~~~ // CHECK:STDERR: fn NonTypeImpls(generic U: type where 7 impls type); diff --git a/toolchain/check/testdata/where_expr/designator.carbon b/toolchain/check/testdata/where_expr/designator.carbon index 5f02d9845867..e44ffffb3dcb 100644 --- a/toolchain/check/testdata/where_expr/designator.carbon +++ b/toolchain/check/testdata/where_expr/designator.carbon @@ -168,6 +168,31 @@ interface Z { // CHECK:STDERR: fn G(unused generic T: Z where A(.Z0) == B and A(B) == B) {} +// --- self_designator_in_both_impls_interface.carbon +library "[[@TEST_NAME]]"; + +interface Z(T: type) {} +interface I(T: type) {} +interface J(T: type) {} +class C; + +fn F(unused generic T: type where C impls (I(.Self) & J(.Self))) {} + +// --- fail_where_without_self_designator_in_type.carbon +library "[[@TEST_NAME]]"; + +interface Z(T: type) {} +interface I {} +interface J(T: type) {} +class C; + +// CHECK:STDERR: fail_where_without_self_designator_in_type.carbon:[[@LINE+4]]:35: error: constraint in `where` clause without a designator; expected `.Self` or a member access like `.M` [WhereWithoutDesignator] +// CHECK:STDERR: fn F(unused generic T: type where C impls type) {} +// CHECK:STDERR: ^~~~~~~~~~~~ +// CHECK:STDERR: +fn F(unused generic T: type where C impls type) {} + + // --- fail_where_without_self_designator_in_one_impls_interface.carbon library "[[@TEST_NAME]]"; @@ -260,15 +285,183 @@ fn F(unused generic T: type where C impls N(.Self)) {} // --- todo_fail_constraint_does_not_constrain_designator.carbon library "[[@TEST_NAME]]"; +interface Y { + fn YY(); +} interface Z {} constraint N(T: type) { require impls Z; } class C; -// TODO: This `.Self` is not actually constrained by `C impls N(.Self)`, so it -// should fail. -fn F(unused generic T: type where C impls N(.Self)) {} +// TODO: This `.Self` is not actually constrained by `C impls N(.Self)`. In +// #7299 we propose that this means `C impls Z` must be satisfied in order to +// identify the facet type. +fn F(generic T: Y where C impls N(.Self)) { + // TODO: This should fail to identify then. + T.YY(); +} + +// --- same_type_designator_in_facet_type.carbon +library "[[@TEST_NAME]]"; + +interface J(T: type) {} + +fn F(generic U: type, unused generic T: type where U == J(.Self)) {} + +fn G(generic U: type, unused generic T: type where (U, ) == (J(.Self), )) {} + +// --- concrete_access_does_constrain_self.carbon +library "[[@TEST_NAME]]"; + +interface I { + let Member: type; +} +interface J {} + +// The `.Member` has a concrete constant value of `()` so the constant value in +// the constraint is `()` which does not have a designator. So we can't look in +// the constant value. + +// Gets a concrete value for `.Member` from a prior constraint. +fn F1(unused generic T: I where .Member = () and .Member == ()) {} + +// Gets a concrete value for `.Member` from a prior constraint. +fn F2(unused generic T: I where .Member = () and .Member impls J) {} + +final impl forall [T: J] T as I where .Member = () {} + +// Gets a concrete value for `.Member` from the final impl. +fn F3(generic _: J where .Self.(I.Member) == ()) {} + +// Gets a concrete value for `.Member` from the final impl. +fn F4(generic _: J where .Self.(I.Member) impls J) {} + +// --- fail_fn_call_returns_type_without_self_in_same_type.carbon +library "[[@TEST_NAME]]"; + +interface I {} +class C; + +eval fn E(generic _: type) -> type { return C; } + +// CHECK:STDERR: fail_fn_call_returns_type_without_self_in_same_type.carbon:[[@LINE+4]]:32: error: constraint in `where` clause without a designator; expected `.Self` or a member access like `.M` [WhereWithoutDesignator] +// CHECK:STDERR: fn F(unused generic T: I where C == E(.Self)) {} +// CHECK:STDERR: ^~~~~~~~~~~~~ +// CHECK:STDERR: +fn F(unused generic T: I where C == E(.Self)) {} + +// --- fail_fn_call_returns_type_without_self_in_impls.carbon +library "[[@TEST_NAME]]"; + +interface I {} +class C; + +eval fn E(generic _: type) -> type { return C; } + +// CHECK:STDERR: fail_fn_call_returns_type_without_self_in_impls.carbon:[[@LINE+4]]:32: error: constraint in `where` clause without a designator; expected `.Self` or a member access like `.M` [WhereWithoutDesignator] +// CHECK:STDERR: fn F(unused generic T: I where C impls E(.Self)) {} +// CHECK:STDERR: ^~~~~~~~~~~~~~~~ +// CHECK:STDERR: +fn F(unused generic T: I where C impls E(.Self)) {} + +// --- fail_fn_call_returns_type_with_self_in_same_type.carbon +library "[[@TEST_NAME]]"; + +interface I {} +class C; + +eval fn E(generic T: type) -> type { return T; } + +// Given `E(.Self)`, it should return `.Self`. However since `.Self` is +// symbolic, the call is not evaluated until `.Self` is replaced with a concrete +// value. As such, we can't tell at the time of checking the facet type if +// `E(.Self)` resolves to an expression containing a designator. + +// CHECK:STDERR: fail_fn_call_returns_type_with_self_in_same_type.carbon:[[@LINE+4]]:32: error: constraint in `where` clause without a designator; expected `.Self` or a member access like `.M` [WhereWithoutDesignator] +// CHECK:STDERR: fn F(unused generic T: I where C == E(.Self)) {} +// CHECK:STDERR: ^~~~~~~~~~~~~ +// CHECK:STDERR: +fn F(unused generic T: I where C == E(.Self)) {} + +// --- fail_fn_call_returns_type_with_self_in_impls.carbon +library "[[@TEST_NAME]]"; + +interface I {} +class C; + +eval fn E(generic T: type) -> type { return T; } + +// Given `E(.Self)`, it should return `.Self`. However since `.Self` is +// symbolic, the call is not evaluated until `.Self` is replaced with a concrete +// value. As such, we can't tell at the time of checking the facet type if +// `E(.Self)` resolves to an expression containing a designator. + +// CHECK:STDERR: fail_fn_call_returns_type_with_self_in_impls.carbon:[[@LINE+4]]:32: error: constraint in `where` clause without a designator; expected `.Self` or a member access like `.M` [WhereWithoutDesignator] +// CHECK:STDERR: fn F(unused generic T: I where C impls E(.Self)) {} +// CHECK:STDERR: ^~~~~~~~~~~~~~~~ +// CHECK:STDERR: +fn F(unused generic T: I where C impls E(.Self)) {} + +// --- alias_of_generic_type.carbon +library "[[@TEST_NAME]]"; + +interface I {} +class C(T: type); + +alias A = C; + +fn F(unused generic T: I where C == A(.Self)) {} + +// --- fail_todo_generic_alias_preserves_self.carbon +library "[[@TEST_NAME]]"; + +interface I {} +class C(T: type); + +// TODO: Once we can write a generic alias, this should pass. + +// CHECK:STDERR: fail_todo_generic_alias_preserves_self.carbon:[[@LINE+4]]:8: error: `alias` declaration cannot have parameters [UnexpectedDeclNameParams] +// CHECK:STDERR: alias A(U: type) = C(U); +// CHECK:STDERR: ^~~~~~~~~ +// CHECK:STDERR: +alias A(U: type) = C(U); + +// CHECK:STDERR: fail_todo_generic_alias_preserves_self.carbon:[[@LINE+8]]:15: error: expected expression [ExpectedExpr] +// CHECK:STDERR: fn F(unused T:! I where C == A(.Self)) {} +// CHECK:STDERR: ^ +// CHECK:STDERR: +// CHECK:STDERR: fail_todo_generic_alias_preserves_self.carbon:[[@LINE+4]]:15: error: semantics TODO: `handle invalid parse trees in `check`` [SemanticsTodo] +// CHECK:STDERR: fn F(unused T:! I where C == A(.Self)) {} +// CHECK:STDERR: ^ +// CHECK:STDERR: +fn F(unused T:! I where C == A(.Self)) {} + +// --- fail_generic_alias_without_self.carbon +library "[[@TEST_NAME]]"; + +interface I {} +class C; + +// CHECK:STDERR: fail_generic_alias_without_self.carbon:[[@LINE+4]]:8: error: `alias` declaration cannot have parameters [UnexpectedDeclNameParams] +// CHECK:STDERR: alias A(U: type) = C; +// CHECK:STDERR: ^~~~~~~~~ +// CHECK:STDERR: +alias A(U: type) = C; + +// The alias returns `C` which does not contain `.Self`. + +// TODO: Once we can write a generic alias, this should become an error because +// there is no constraint against `.Self`. +// CHECK:STDERR: fail_generic_alias_without_self.carbon:[[@LINE+8]]:15: error: expected expression [ExpectedExpr] +// CHECK:STDERR: fn F(unused T:! I where C == A(.Self)) {} +// CHECK:STDERR: ^ +// CHECK:STDERR: +// CHECK:STDERR: fail_generic_alias_without_self.carbon:[[@LINE+4]]:15: error: semantics TODO: `handle invalid parse trees in `check`` [SemanticsTodo] +// CHECK:STDERR: fn F(unused T:! I where C == A(.Self)) {} +// CHECK:STDERR: ^ +// CHECK:STDERR: +fn F(unused T:! I where C == A(.Self)) {} // CHECK:STDOUT: --- success.carbon // CHECK:STDOUT: