Decouple associated constants from let (#5973)

Decouples associated constants from being special cased in let handlers.
Enforces associated constant grammar restrictions in parsing instead of
checking.

Closes #5411

Author: elkalt

toolchain/check/handle_binding_pattern.cpp

@@ -14,6 +14,7 @@
 #include "toolchain/check/type.h"
 #include "toolchain/check/type_completion.h"
 #include "toolchain/diagnostics/format_providers.h"
+#include "toolchain/parse/node_ids.h"
 #include "toolchain/sem_ir/ids.h"
 #include "toolchain/sem_ir/inst.h"
 #include "toolchain/sem_ir/pattern.h"
@@ -87,61 +88,6 @@ static auto HandleAnyBindingPattern(Context& context, Parse::NodeId node_id,
     context.emitter().Emit(node_id, SelfOutsideImplicitParamList);
   }
 
-  // A binding in an interface scope declares an associated constant, not a
-  // true binding, so we handle it separately.
-  if (auto parent_interface_decl =
-          context.scope_stack().GetCurrentScopeAs<SemIR::InterfaceDecl>();
-      parent_interface_decl.has_value()) {
-    // TODO: diagnose this during parsing, to avoid near-duplicate error
-    // messages.
-    if (!is_generic) {
-      CARBON_DIAGNOSTIC(ExpectedSymbolicBindingInAssociatedConstant, Error,
-                        "found runtime binding pattern in associated constant "
-                        "declaration; expected a `:!` binding");
-      context.emitter().Emit(node_id,
-                             ExpectedSymbolicBindingInAssociatedConstant);
-      context.node_stack().Push(node_id, SemIR::ErrorInst::InstId);
-      return true;
-    }
-    if (name_id == SemIR::NameId::Underscore) {
-      // The action item here may be to document this as not allowed, and
-      // add a proper diagnostic.
-      context.TODO(node_id, "_ used as associated constant name");
-    }
-    cast_type_id = AsCompleteType(context, cast_type_id, type_node, [&] {
-      CARBON_DIAGNOSTIC(IncompleteTypeInAssociatedConstantDecl, Error,
-                        "associated constant has incomplete type {0}",
-                        SemIR::TypeId);
-      return context.emitter().Build(
-          type_node, IncompleteTypeInAssociatedConstantDecl, cast_type_id);
-    });
-    if (is_template) {
-      CARBON_DIAGNOSTIC(TemplateBindingInAssociatedConstantDecl, Error,
-                        "associated constant has `template` binding");
-      context.emitter().Emit(type_node,
-                             TemplateBindingInAssociatedConstantDecl);
-    }
-
-    SemIR::AssociatedConstantDecl assoc_const_decl = {
-        .type_id = cast_type_id,
-        .assoc_const_id = SemIR::AssociatedConstantId::None,
-        .decl_block_id = SemIR::InstBlockId::None};
-    auto decl_id = AddPlaceholderInstInNoBlock(
-        context,
-        context.parse_tree().As<Parse::CompileTimeBindingPatternId>(node_id),
-        assoc_const_decl);
-    assoc_const_decl.assoc_const_id = context.associated_constants().Add(
-        {.name_id = name_id,
-         .parent_scope_id = context.scope_stack().PeekNameScopeId(),
-         .decl_id = decl_id,
-         .generic_id = SemIR::GenericId::None,
-         .default_value_id = SemIR::InstId::None});
-    ReplaceInstBeforeConstantUse(context, decl_id, assoc_const_decl);
-
-    context.node_stack().Push(node_id, decl_id);
-    return true;
-  }
-
   // Allocate an instruction of the appropriate kind, linked to the name for
   // error locations.
   switch (context.full_pattern_stack().CurrentKind()) {
@@ -298,19 +244,20 @@ auto HandleParseNode(Context& context,
   context.scope_stack().Pop();
 
   auto node_kind = Parse::NodeKind::CompileTimeBindingPattern;
-
   const DeclIntroducerState& introducer =
       context.decl_introducer_state_stack().innermost();
   if (introducer.kind == Lex::TokenKind::Let) {
     // Disallow `let` outside of function and interface definitions.
     // TODO: Find a less brittle way of doing this. A `scope_inst_id` of `None`
     // can represent a block scope, but is also used for other kinds of scopes
-    // that aren't necessarily part of an interface or function decl.
+    // that aren't necessarily part of a function decl.
+    // We don't need to check if the scope is an interface here as this is
+    // already caught in the parse phase by the separated associated constant
+    // logic.
     auto scope_inst_id = context.scope_stack().PeekInstId();
     if (scope_inst_id.has_value()) {
       auto scope_inst = context.insts().Get(scope_inst_id);
-      if (!scope_inst.Is<SemIR::InterfaceDecl>() &&
-          !scope_inst.Is<SemIR::FunctionDecl>()) {
+      if (!scope_inst.Is<SemIR::FunctionDecl>()) {
         context.TODO(
             node_id,
             "`let` compile time binding outside function or interface");
@@ -322,6 +269,47 @@ auto HandleParseNode(Context& context,
   return HandleAnyBindingPattern(context, node_id, node_kind);
 }
 
+auto HandleParseNode(Context& context,
+                     Parse::AssociatedConstantNameAndTypeId node_id) -> bool {
+  auto [type_node, parsed_type_id] = context.node_stack().PopExprWithNodeId();
+  auto [cast_type_inst_id, cast_type_id] =
+      ExprAsType(context, type_node, parsed_type_id);
+
+  EndSubpatternAsExpr(context, cast_type_inst_id);
+
+  auto [name_node, name_id] = context.node_stack().PopNameWithNodeId();
+
+  if (name_id == SemIR::NameId::Underscore) {
+    // The action item here may be to document this as not allowed, and
+    // add a proper diagnostic.
+    context.TODO(node_id, "_ used as associated constant name");
+  }
+  cast_type_id = AsCompleteType(context, cast_type_id, type_node, [&] {
+    CARBON_DIAGNOSTIC(IncompleteTypeInAssociatedConstantDecl, Error,
+                      "associated constant has incomplete type {0}",
+                      SemIR::TypeId);
+    return context.emitter().Build(
+        type_node, IncompleteTypeInAssociatedConstantDecl, cast_type_id);
+  });
+
+  SemIR::AssociatedConstantDecl assoc_const_decl = {
+      .type_id = cast_type_id,
+      .assoc_const_id = SemIR::AssociatedConstantId::None,
+      .decl_block_id = SemIR::InstBlockId::None};
+  auto decl_id =
+      AddPlaceholderInstInNoBlock(context, node_id, assoc_const_decl);
+  assoc_const_decl.assoc_const_id = context.associated_constants().Add(
+      {.name_id = name_id,
+       .parent_scope_id = context.scope_stack().PeekNameScopeId(),
+       .decl_id = decl_id,
+       .generic_id = SemIR::GenericId::None,
+       .default_value_id = SemIR::InstId::None});
+  ReplaceInstBeforeConstantUse(context, decl_id, assoc_const_decl);
+
+  context.node_stack().Push(node_id, decl_id);
+  return true;
+}
+
 auto HandleParseNode(Context& context, Parse::FieldNameAndTypeId node_id)
     -> bool {
   auto [type_node, parsed_type_id] = context.node_stack().PopExprWithNodeId();

toolchain/check/handle_let_and_var.cpp

@@ -18,6 +18,7 @@
 #include "toolchain/diagnostics/diagnostic_emitter.h"
 #include "toolchain/diagnostics/format_providers.h"
 #include "toolchain/lex/token_kind.h"
+#include "toolchain/parse/node_ids.h"
 #include "toolchain/parse/node_kind.h"
 #include "toolchain/parse/typed_nodes.h"
 #include "toolchain/sem_ir/ids.h"
@@ -29,15 +30,6 @@
 
 namespace Carbon::Check {
 
-// Handles the start of a declaration of an associated constant.
-static auto StartAssociatedConstant(Context& context) -> void {
-  // An associated constant is always generic.
-  StartGenericDecl(context);
-  // Collect the declarations nested in the associated constant in a decl
-  // block. This is popped by FinishAssociatedConstantDecl.
-  context.inst_block_stack().Push();
-}
-
 // Handles the end of the declaration region of an associated constant. This is
 // called at the `=` or the `;` of the declaration, whichever comes first.
 static auto EndAssociatedConstantDeclRegion(Context& context,
@@ -95,10 +87,16 @@ static auto HandleIntroducer(Context& context, Parse::NodeId node_id) -> bool {
 }
 
 auto HandleParseNode(Context& context, Parse::LetIntroducerId node_id) -> bool {
-  if (context.scope_stack().GetCurrentScopeAs<SemIR::InterfaceDecl>()) {
-    StartAssociatedConstant(context);
-  }
+  return HandleIntroducer<Lex::TokenKind::Let>(context, node_id);
+}
 
+auto HandleParseNode(Context& context,
+                     Parse::AssociatedConstantIntroducerId node_id) -> bool {
+  // An associated constant is always generic.
+  StartGenericDecl(context);
+  // Collect the declarations nested in the associated constant in a decl
+  // block. This is popped by FinishAssociatedConstantDecl.
+  context.inst_block_stack().Push();
   return HandleIntroducer<Lex::TokenKind::Let>(context, node_id);
 }
 
@@ -177,14 +175,18 @@ static auto HandleInitializer(Context& context, Parse::NodeId node_id) -> bool {
 
 auto HandleParseNode(Context& context, Parse::LetInitializerId node_id)
     -> bool {
-  if (auto interface_decl =
-          context.scope_stack().GetCurrentScopeAs<SemIR::InterfaceDecl>()) {
-    auto generic_id =
-        EndAssociatedConstantDeclRegion(context, interface_decl->interface_id);
+  return HandleInitializer(context, node_id);
+}
 
-    // Start building the definition region of the constant.
-    StartGenericDefinition(context, generic_id);
-  }
+auto HandleParseNode(Context& context,
+                     Parse::AssociatedConstantInitializerId node_id) -> bool {
+  auto interface_decl =
+      context.scope_stack().GetCurrentScopeAs<SemIR::InterfaceDecl>();
+  auto generic_id =
+      EndAssociatedConstantDeclRegion(context, interface_decl->interface_id);
+
+  // Start building the definition region of the constant.
+  StartGenericDefinition(context, generic_id);
 
   return HandleInitializer(context, node_id);
 }
@@ -232,11 +234,10 @@ static auto HandleDecl(Context& context) -> DeclInfo {
   } else {
     // For an associated constant declaration, handle the completed declaration
     // now. We will have done this at the `=` if there was an initializer.
-    if (IntroducerTokenKind == Lex::TokenKind::Let) {
-      if (auto interface_decl =
-              context.scope_stack().GetCurrentScopeAs<SemIR::InterfaceDecl>()) {
-        EndAssociatedConstantDeclRegion(context, interface_decl->interface_id);
-      }
+    if (IntroducerNodeKind == Parse::NodeKind::AssociatedConstantIntroducer) {
+      auto interface_decl =
+          context.scope_stack().GetCurrentScopeAs<SemIR::InterfaceDecl>();
+      EndAssociatedConstantDeclRegion(context, interface_decl->interface_id);
     }
 
     EndFullPattern(context);
@@ -261,23 +262,59 @@ static auto HandleDecl(Context& context) -> DeclInfo {
   return decl_info;
 }
 
-// Finishes an associated constant declaration. This is called at the `;` to
-// perform any final steps. The `AssociatedConstantDecl` instruction and the
-// corresponding `AssociatedConstant` entity are built as part of handling the
-// binding pattern, but we still need to finish building the `Generic` object
-// and attach the default value, if any is specified.
-static auto FinishAssociatedConstant(Context& context, Parse::LetDeclId node_id,
-                                     SemIR::InterfaceId interface_id,
-                                     DeclInfo& decl_info) -> void {
+auto HandleParseNode(Context& context, Parse::LetDeclId node_id) -> bool {
+  auto decl_info =
+      HandleDecl<Lex::TokenKind::Let, Parse::NodeKind::LetIntroducer,
+                 Parse::NodeKind::LetInitializer>(context);
+
+  LimitModifiersOnDecl(
+      context, decl_info.introducer,
+      KeywordModifierSet::Access | KeywordModifierSet::Interface);
+
+  // Diagnose interface modifiers given that we're not building an associated
+  // constant. We use this rather than `LimitModifiersOnDecl` to get a more
+  // specific error.
+  RequireDefaultFinalOnlyInInterfaces(context, decl_info.introducer,
+                                      std::nullopt);
+
+  if (decl_info.init_id.has_value()) {
+    LocalPatternMatch(context, decl_info.pattern_id, decl_info.init_id);
+  } else {
+    CARBON_DIAGNOSTIC(
+        ExpectedInitializerAfterLet, Error,
+        "expected `=`; `let` declaration must have an initializer");
+    context.emitter().Emit(LocIdForDiagnostics::TokenOnly(node_id),
+                           ExpectedInitializerAfterLet);
+  }
+  return true;
+}
+
+auto HandleParseNode(Context& context, Parse::AssociatedConstantDeclId node_id)
+    -> bool {
+  auto decl_info =
+      HandleDecl<Lex::TokenKind::Let,
+                 Parse::NodeKind::AssociatedConstantIntroducer,
+                 Parse::NodeKind::AssociatedConstantInitializer>(context);
+
+  LimitModifiersOnDecl(
+      context, decl_info.introducer,
+      KeywordModifierSet::Access | KeywordModifierSet::Interface);
+
+  auto interface_scope =
+      context.scope_stack().GetCurrentScopeAs<SemIR::InterfaceDecl>();
+  // The `AssociatedConstantDecl` instruction and the
+  // corresponding `AssociatedConstant` entity are built as part of handling the
+  // binding pattern, but we still need to finish building the `Generic` object
+  // and attach the default value, if any is specified.
   if (decl_info.pattern_id == SemIR::ErrorInst::InstId) {
     context.name_scopes()
-        .Get(context.interfaces().Get(interface_id).scope_id)
+        .Get(context.interfaces().Get(interface_scope->interface_id).scope_id)
         .set_has_error();
     if (decl_info.init_id.has_value()) {
       DiscardGenericDecl(context);
     }
     context.inst_block_stack().Pop();
-    return;
+    return true;
   }
   auto decl = context.insts().GetAs<SemIR::AssociatedConstantDecl>(
       decl_info.pattern_id);
@@ -306,41 +343,6 @@ static auto FinishAssociatedConstant(Context& context, Parse::LetDeclId node_id,
   ReplaceInstPreservingConstantValue(context, decl_info.pattern_id, decl);
 
   context.inst_block_stack().AddInstId(decl_info.pattern_id);
-}
-
-auto HandleParseNode(Context& context, Parse::LetDeclId node_id) -> bool {
-  auto decl_info =
-      HandleDecl<Lex::TokenKind::Let, Parse::NodeKind::LetIntroducer,
-                 Parse::NodeKind::LetInitializer>(context);
-
-  LimitModifiersOnDecl(
-      context, decl_info.introducer,
-      KeywordModifierSet::Access | KeywordModifierSet::Interface);
-
-  // At interface scope, we are forming an associated constant, which has
-  // different rules.
-  if (auto interface_scope =
-          context.scope_stack().GetCurrentScopeAs<SemIR::InterfaceDecl>()) {
-    FinishAssociatedConstant(context, node_id, interface_scope->interface_id,
-                             decl_info);
-    return true;
-  }
-
-  // Diagnose interface modifiers given that we're not building an associated
-  // constant. We use this rather than `LimitModifiersOnDecl` to get a more
-  // specific error.
-  RequireDefaultFinalOnlyInInterfaces(context, decl_info.introducer,
-                                      std::nullopt);
-
-  if (decl_info.init_id.has_value()) {
-    LocalPatternMatch(context, decl_info.pattern_id, decl_info.init_id);
-  } else {
-    CARBON_DIAGNOSTIC(
-        ExpectedInitializerAfterLet, Error,
-        "expected `=`; `let` declaration must have an initializer");
-    context.emitter().Emit(LocIdForDiagnostics::TokenOnly(node_id),
-                           ExpectedInitializerAfterLet);
-  }
   return true;
 }
 

toolchain/check/handle_pattern_list.cpp

@@ -74,15 +74,6 @@ auto HandleParseNode(Context& context, Parse::TuplePatternId node_id) -> bool {
   context.node_stack()
       .PopAndDiscardSoloNodeId<Parse::NodeKind::TuplePatternStart>();
 
-  if (context.scope_stack().GetCurrentScopeAs<SemIR::InterfaceDecl>()) {
-    CARBON_DIAGNOSTIC(ExpectedSingleBindingInAssociatedConstant, Error,
-                      "found tuple pattern in associated constant declaration; "
-                      "expected symbolic binding pattern");
-    context.emitter().Emit(node_id, ExpectedSingleBindingInAssociatedConstant);
-    context.node_stack().Push(node_id, SemIR::ErrorInst::InstId);
-    EndSubpatternAsNonExpr(context);
-    return true;
-  }
   const auto& inst_block = context.inst_blocks().Get(refs_id);
   llvm::SmallVector<SemIR::InstId> type_inst_ids;
   type_inst_ids.reserve(inst_block.size());

toolchain/check/node_stack.h

@@ -457,6 +457,8 @@ class NodeStack {
       case Parse::NodeKind::InterfaceIntroducer:
       case Parse::NodeKind::LetInitializer:
       case Parse::NodeKind::LetIntroducer:
+      case Parse::NodeKind::AssociatedConstantIntroducer:
+      case Parse::NodeKind::AssociatedConstantInitializer:
       case Parse::NodeKind::ReturnStatementStart:
       case Parse::NodeKind::StructLiteralStart:
       case Parse::NodeKind::StructTypeLiteralField: