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

@@ -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);
 

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<SemIR::TypeType>(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<SemIR::Call>(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<SemIR::FacetType>(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<SemIR::RequirementEquivalent>(context, node_id,
-                                            {.lhs_id = lhs, .rhs_id = rhs}));
+      AddInst<SemIR::RequirementEquivalent>(
+          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<SemIR::FacetType>(
-                const_rhs)) {
-      const auto& info = context.facet_types().Get(facet_type->facet_type_id);
-      for (auto extend : llvm::concat<SemIR::SpecificId>(
-               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;
     }
   }
 

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;
 }

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

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) {}
 

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);
 

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: