Track params in the parser (#4777)

This change splits `NodeKind::IdentifierName` into separate node kinds depending on whether the identifier is followed by parameters, and similarly splits `NameQualifier` based on whether the qualifier has parameters. This enables us to only push a pattern block when it's actually needed, rather than "defensively" pushing one when it might be needed. --------- Co-authored-by: Jon Ross-Perkins <[email protected]>
carbon-language · Jan 10, 2025 · 4f10735 · 4f10735
1 parent 8f685b6
commit 4f10735
Show file tree

Hide file tree

Showing 297 changed files with 942 additions and 905 deletions.
diff --git a/toolchain/check/handle_alias.cpp b/toolchain/check/handle_alias.cpp
@@ -16,11 +16,6 @@ auto HandleParseNode(Context& context, Parse::AliasIntroducerId /*node_id*/)
     -> bool {
   context.decl_introducer_state_stack().Push<Lex::TokenKind::Alias>();
   context.decl_name_stack().PushScopeAndStartName();
-
-  // Push a pattern block to handle parameters of the alias declaration.
-  // TODO: Disallow these in parse, instead of check, so we don't have to do
-  // this.
-  context.pattern_block_stack().Push();
   return true;
 }
 

diff --git a/toolchain/check/handle_class.cpp b/toolchain/check/handle_class.cpp
@@ -42,10 +42,6 @@ auto HandleParseNode(Context& context, Parse::ClassIntroducerId node_id)
   context.decl_name_stack().PushScopeAndStartName();
   // This class is potentially generic.
   StartGenericDecl(context);
-  // Push a pattern block for the signature (if any) of the first NameComponent.
-  // TODO: Instead use a separate parse node kind for an identifier that's
-  // followed by a pattern, and push a pattern block when handling it.
-  context.pattern_block_stack().Push();
   return true;
 }
 

diff --git a/toolchain/check/handle_export.cpp b/toolchain/check/handle_export.cpp
@@ -18,9 +18,6 @@ auto HandleParseNode(Context& context, Parse::ExportIntroducerId /*node_id*/)
   context.decl_introducer_state_stack().Push<Lex::TokenKind::Export>();
   // TODO: Probably need to update DeclNameStack to restrict to only namespaces.
   context.decl_name_stack().PushScopeAndStartName();
-  // The parser supports patterns after `export`, so we need a pattern block
-  // to handle them.
-  context.pattern_block_stack().Push();
   return true;
 }
 

diff --git a/toolchain/check/handle_function.cpp b/toolchain/check/handle_function.cpp
@@ -36,8 +36,6 @@ auto HandleParseNode(Context& context, Parse::FunctionIntroducerId node_id)
   context.decl_name_stack().PushScopeAndStartName();
   // The function is potentially generic.
   StartGenericDecl(context);
-  // Start a new pattern block for the signature.
-  context.pattern_block_stack().Push();
   return true;
 }
 

diff --git a/toolchain/check/handle_interface.cpp b/toolchain/check/handle_interface.cpp
@@ -25,10 +25,6 @@ auto HandleParseNode(Context& context, Parse::InterfaceIntroducerId node_id)
   context.decl_name_stack().PushScopeAndStartName();
   // This interface is potentially generic.
   StartGenericDecl(context);
-  // Push a pattern block for the signature (if any) of the first NameComponent.
-  // TODO: Instead use a separate parse node kind for an identifier that's
-  // followed by a pattern, and push a pattern block when handling it.
-  context.pattern_block_stack().Push();
   return true;
 }
 

diff --git a/toolchain/check/handle_name.cpp b/toolchain/check/handle_name.cpp
@@ -123,7 +123,8 @@ static auto HandleNameAsExpr(Context& context, Parse::NodeId node_id,
       {.type_id = type_id, .name_id = name_id, .value_id = result.inst_id});
 }
 
-auto HandleParseNode(Context& context, Parse::IdentifierNameId node_id)
+static auto HandleIdentifierName(Context& context,
+                                 Parse::AnyNonExprIdentifierNameId node_id)
     -> bool {
   // The parent is responsible for binding the name.
   auto name_id = GetIdentifierAsName(context, node_id);
@@ -133,6 +134,18 @@ auto HandleParseNode(Context& context, Parse::IdentifierNameId node_id)
   return true;
 }
 
+auto HandleParseNode(Context& context,
+                     Parse::IdentifierNameNotBeforeParamsId node_id) -> bool {
+  return HandleIdentifierName(context, node_id);
+}
+
+auto HandleParseNode(Context& context,
+                     Parse::IdentifierNameBeforeParamsId node_id) -> bool {
+  // Push a pattern block stack entry to handle the parameter pattern.
+  context.pattern_block_stack().Push();
+  return HandleIdentifierName(context, node_id);
+}
+
 auto HandleParseNode(Context& context, Parse::IdentifierNameExprId node_id)
     -> bool {
   auto name_id = GetIdentifierAsName(context, node_id);
@@ -172,13 +185,15 @@ auto HandleParseNode(Context& context, Parse::SelfValueNameExprId node_id)
   return true;
 }
 
-auto HandleParseNode(Context& context, Parse::NameQualifierId /*node_id*/)
-    -> bool {
+auto HandleParseNode(Context& context,
+                     Parse::NameQualifierWithParamsId /*node_id*/) -> bool {
+  context.decl_name_stack().ApplyNameQualifier(PopNameComponent(context));
+  return true;
+}
+
+auto HandleParseNode(Context& context,
+                     Parse::NameQualifierWithoutParamsId /*node_id*/) -> bool {
   context.decl_name_stack().ApplyNameQualifier(PopNameComponent(context));
-  // Push a pattern block for the signature (if any) of the next NameComponent.
-  // TODO: Instead use a separate parse node kind for an identifier that's
-  // followed by a pattern, and push a pattern block when handling it.
-  context.pattern_block_stack().Push();
   return true;
 }
 

diff --git a/toolchain/check/handle_namespace.cpp b/toolchain/check/handle_namespace.cpp
@@ -19,11 +19,6 @@ auto HandleParseNode(Context& context, Parse::NamespaceStartId /*node_id*/)
   // Optional modifiers and the name follow.
   context.decl_introducer_state_stack().Push<Lex::TokenKind::Namespace>();
   context.decl_name_stack().PushScopeAndStartName();
-
-  // Push a pattern block to handle parameters of the namespace declaration.
-  // TODO: Disallow these in parse, instead of check, so we don't have to do
-  // this.
-  context.pattern_block_stack().Push();
   return true;
 }
 

diff --git a/toolchain/check/inst_block_stack.h b/toolchain/check/inst_block_stack.h
@@ -54,7 +54,7 @@ class InstBlockStack {
 
   // Adds the given instruction ID to the block at the top of the stack.
   auto AddInstId(SemIR::InstId inst_id) -> void {
-    CARBON_CHECK(!empty(), "no current block");
+    CARBON_CHECK(!empty(), "{0} has no current block", name_);
     insts_stack_.AppendToTop(inst_id);
   }
 
@@ -74,7 +74,7 @@ class InstBlockStack {
 
   // Runs verification that the processing cleanly finished.
   auto VerifyOnFinish() const -> void {
-    CARBON_CHECK(empty(), "{0}", id_stack_.size());
+    CARBON_CHECK(empty(), "{0} still has {1} entries", name_, id_stack_.size());
   }
 
   auto empty() const -> bool { return id_stack_.empty(); }

diff --git a/toolchain/check/name_component.cpp b/toolchain/check/name_component.cpp
@@ -43,11 +43,19 @@ auto PopNameComponent(Context& context, SemIR::InstId return_slot_pattern_id)
     implicit_param_patterns_id = SemIR::InstBlockId::Invalid;
   }
 
-  auto call_params_id =
-      CalleePatternMatch(context, *implicit_param_patterns_id,
-                         *param_patterns_id, return_slot_pattern_id);
+  auto call_params_id = SemIR::InstBlockId::Invalid;
+  auto pattern_block_id = SemIR::InstBlockId::Invalid;
+  if (param_patterns_id->is_valid() || implicit_param_patterns_id->is_valid()) {
+    call_params_id =
+        CalleePatternMatch(context, *implicit_param_patterns_id,
+                           *param_patterns_id, return_slot_pattern_id);
+    pattern_block_id = context.pattern_block_stack().Pop();
+  }
+
+  auto [name_loc_id, name_id] =
+      context.node_stack()
+          .PopWithNodeId<Parse::NodeCategory::NonExprIdentifierName>();
 
-  auto [name_loc_id, name_id] = context.node_stack().PopNameWithNodeId();
   return {
       .name_loc_id = name_loc_id,
       .name_id = name_id,
@@ -59,7 +67,7 @@ auto PopNameComponent(Context& context, SemIR::InstId return_slot_pattern_id)
       .param_patterns_id = *param_patterns_id,
       .call_params_id = call_params_id,
       .return_slot_pattern_id = return_slot_pattern_id,
-      .pattern_block_id = context.pattern_block_stack().Pop(),
+      .pattern_block_id = pattern_block_id,
   };
 }
 

diff --git a/toolchain/check/name_component.h b/toolchain/check/name_component.h
@@ -49,13 +49,14 @@ struct NameComponent {
   SemIR::InstBlockId pattern_block_id;
 };
 
-// Pop a name component from the node stack and pattern block stack.
+// Pops a name component from the node stack (and pattern block stack, if it has
+// parameters).
 auto PopNameComponent(Context& context, SemIR::InstId return_slot_pattern_id =
                                             SemIR::InstId::Invalid)
     -> NameComponent;
 
-// Pop the name of a declaration from the node stack and pattern block stack,
-// and diagnose if it has parameters.
+// Equivalent to PopNameComponent, but also diagnoses if the name component has
+// parameters.
 auto PopNameComponentWithoutParams(Context& context, Lex::TokenKind introducer)
     -> NameComponent;
 

diff --git a/toolchain/check/node_stack.h b/toolchain/check/node_stack.h
@@ -386,7 +386,8 @@ class NodeStack {
     // `TuplePattern` produces an `InstBlockId`.
     set_id_if_category_is(Parse::NodeCategory::Expr,
                           Id::KindFor<SemIR::InstId>());
-    set_id_if_category_is(Parse::NodeCategory::MemberName,
+    set_id_if_category_is(Parse::NodeCategory::MemberName |
+                              Parse::NodeCategory::NonExprIdentifierName,
                           Id::KindFor<SemIR::NameId>());
     set_id_if_category_is(Parse::NodeCategory::ImplAs,
                           Id::KindFor<SemIR::InstId>());
@@ -521,7 +522,8 @@ class NodeStack {
         case Parse::NodeKind::ForStatement:
         case Parse::NodeKind::FunctionDecl:
         case Parse::NodeKind::FunctionDefinition:
-        case Parse::NodeKind::IdentifierName:
+        case Parse::NodeKind::IdentifierNameNotBeforeParams:
+        case Parse::NodeKind::IdentifierNameBeforeParams:
         case Parse::NodeKind::IdentifierNameExpr:
         case Parse::NodeKind::IfConditionStart:
         case Parse::NodeKind::IfExprElse:
@@ -594,7 +596,8 @@ class NodeStack {
         case Parse::NodeKind::NamedConstraintDefinition:
         case Parse::NodeKind::NamedConstraintDefinitionStart:
         case Parse::NodeKind::NamedConstraintIntroducer:
-        case Parse::NodeKind::NameQualifier:
+        case Parse::NodeKind::NameQualifierWithParams:
+        case Parse::NodeKind::NameQualifierWithoutParams:
         case Parse::NodeKind::Namespace:
         case Parse::NodeKind::NamespaceStart:
         case Parse::NodeKind::PackageDecl:

diff --git a/toolchain/language_server/handle_document_symbol.cpp b/toolchain/language_server/handle_document_symbol.cpp
@@ -13,19 +13,25 @@
 
 namespace Carbon::LanguageServer {
 
-// Returns the text of first child of kind Parse::NodeKind::IdentifierName.
+// Returns the text of first child of kind IdentifierNameBeforeParams or
+// IdentifierNameNotBeforeParams.
 static auto GetIdentifierName(const SharedValueStores& value_stores,
                               const Lex::TokenizedBuffer& tokens,
                               const Parse::TreeAndSubtrees& tree_and_subtrees,
                               Parse::NodeId node)
     -> std::optional<llvm::StringRef> {
   for (auto child : tree_and_subtrees.children(node)) {
-    if (tree_and_subtrees.tree().node_kind(child) ==
-        Parse::NodeKind::IdentifierName) {
-      auto token = tree_and_subtrees.tree().node_token(child);
-      if (tokens.GetKind(token) == Lex::TokenKind::Identifier) {
-        return value_stores.identifiers().Get(tokens.GetIdentifier(token));
+    switch (tree_and_subtrees.tree().node_kind(child)) {
+      case Parse::NodeKind::IdentifierNameBeforeParams:
+      case Parse::NodeKind::IdentifierNameNotBeforeParams: {
+        auto token = tree_and_subtrees.tree().node_token(child);
+        if (tokens.GetKind(token) == Lex::TokenKind::Identifier) {
+          return value_stores.identifiers().Get(tokens.GetIdentifier(token));
+        }
+        break;
       }
+      default:
+        break;
     }
   }
   return std::nullopt;

diff --git a/toolchain/parse/handle_binding_pattern.cpp b/toolchain/parse/handle_binding_pattern.cpp
@@ -40,7 +40,7 @@ auto HandleBindingPattern(Context& context) -> void {
   // The first item should be an identifier or `self`.
   bool has_name = false;
   if (auto identifier = context.ConsumeIf(Lex::TokenKind::Identifier)) {
-    context.AddLeafNode(NodeKind::IdentifierName, *identifier);
+    context.AddLeafNode(NodeKind::IdentifierNameNotBeforeParams, *identifier);
     has_name = true;
   } else if (auto self =
                  context.ConsumeIf(Lex::TokenKind::SelfValueIdentifier)) {
@@ -51,8 +51,8 @@ auto HandleBindingPattern(Context& context) -> void {
   }
   if (!has_name) {
     // Add a placeholder for the name.
-    context.AddLeafNode(NodeKind::IdentifierName, *context.position(),
-                        /*has_error=*/true);
+    context.AddLeafNode(NodeKind::IdentifierNameNotBeforeParams,
+                        *context.position(), /*has_error=*/true);
     on_error(/*expected_name=*/true);
   }
 

diff --git a/toolchain/parse/handle_choice.cpp b/toolchain/parse/handle_choice.cpp
@@ -50,8 +50,8 @@ auto HandleChoiceAlternative(Context& context) -> void {
 
   context.PushState(State::ChoiceAlternativeFinish);
 
-  if (!context.ConsumeAndAddLeafNodeIf(Lex::TokenKind::Identifier,
-                                       NodeKind::IdentifierName)) {
+  auto token = context.ConsumeIf(Lex::TokenKind::Identifier);
+  if (!token) {
     if (!state.has_error) {
       CARBON_DIAGNOSTIC(ExpectedChoiceAlternativeName, Error,
                         "expected choice alternative name");
@@ -67,7 +67,10 @@ auto HandleChoiceAlternative(Context& context) -> void {
   }
 
   if (context.PositionIs(Lex::TokenKind::OpenParen)) {
+    context.AddLeafNode(NodeKind::IdentifierNameBeforeParams, *token);
     context.PushState(State::PatternListAsTuple);
+  } else {
+    context.AddLeafNode(NodeKind::IdentifierNameNotBeforeParams, *token);
   }
 }
 

diff --git a/toolchain/parse/handle_decl_name_and_params.cpp b/toolchain/parse/handle_decl_name_and_params.cpp
@@ -34,29 +34,31 @@ auto HandleDeclNameAndParams(Context& context) -> void {
     return;
   }
 
-  context.AddLeafNode(NodeKind::IdentifierName, *identifier);
-
   switch (context.PositionKind()) {
     case Lex::TokenKind::Period:
-      context.AddNode(NodeKind::NameQualifier,
+      context.AddLeafNode(NodeKind::IdentifierNameNotBeforeParams, *identifier);
+      context.AddNode(NodeKind::NameQualifierWithoutParams,
                       context.ConsumeChecked(Lex::TokenKind::Period),
                       state.has_error);
       context.PushState(State::DeclNameAndParams);
       break;
 
     case Lex::TokenKind::OpenSquareBracket:
+      context.AddLeafNode(NodeKind::IdentifierNameBeforeParams, *identifier);
       state.state = State::DeclNameAndParamsAfterImplicit;
       context.PushState(state);
       context.PushState(State::PatternListAsImplicit);
       break;
 
     case Lex::TokenKind::OpenParen:
+      context.AddLeafNode(NodeKind::IdentifierNameBeforeParams, *identifier);
       state.state = State::DeclNameAndParamsAfterParams;
       context.PushState(state);
       context.PushState(State::PatternListAsTuple);
       break;
 
     default:
+      context.AddLeafNode(NodeKind::IdentifierNameNotBeforeParams, *identifier);
       break;
   }
 }
@@ -82,7 +84,8 @@ auto HandleDeclNameAndParamsAfterParams(Context& context) -> void {
   auto state = context.PopState();
 
   if (auto period = context.ConsumeIf(Lex::TokenKind::Period)) {
-    context.AddNode(NodeKind::NameQualifier, *period, state.has_error);
+    context.AddNode(NodeKind::NameQualifierWithParams, *period,
+                    state.has_error);
     context.PushState(State::DeclNameAndParams);
   }
 }

diff --git a/toolchain/parse/handle_expr.cpp b/toolchain/parse/handle_expr.cpp
@@ -179,8 +179,9 @@ auto HandleExprInPostfix(Context& context) -> void {
       // For periods, we look at the next token to form a designator like
       // `.Member` or `.Self`.
       auto period = context.Consume();
-      if (context.ConsumeAndAddLeafNodeIf(Lex::TokenKind::Identifier,
-                                          NodeKind::IdentifierName)) {
+      if (context.ConsumeAndAddLeafNodeIf(
+              Lex::TokenKind::Identifier,
+              NodeKind::IdentifierNameNotBeforeParams)) {
         // OK, `.` identifier.
       } else if (context.ConsumeAndAddLeafNodeIf(
                      Lex::TokenKind::SelfTypeIdentifier,
@@ -193,8 +194,8 @@ auto HandleExprInPostfix(Context& context) -> void {
                                ExpectedIdentifierOrSelfAfterPeriod);
         // Only consume if it is a number or word.
         if (context.PositionKind().is_keyword()) {
-          context.AddLeafNode(NodeKind::IdentifierName, context.Consume(),
-                              /*has_error=*/true);
+          context.AddLeafNode(NodeKind::IdentifierNameNotBeforeParams,
+                              context.Consume(), /*has_error=*/true);
         } else if (context.PositionIs(Lex::TokenKind::IntLiteral)) {
           context.AddInvalidParse(context.Consume());
         } else {

diff --git a/toolchain/parse/handle_period.cpp b/toolchain/parse/handle_period.cpp
@@ -19,8 +19,9 @@ static auto HandlePeriodOrArrow(Context& context, NodeKind node_kind,
   auto dot = context.ConsumeChecked(is_arrow ? Lex::TokenKind::MinusGreater
                                              : Lex::TokenKind::Period);
 
-  if (context.ConsumeAndAddLeafNodeIf(Lex::TokenKind::Identifier,
-                                      NodeKind::IdentifierName)) {
+  if (context.ConsumeAndAddLeafNodeIf(
+          Lex::TokenKind::Identifier,
+          NodeKind::IdentifierNameNotBeforeParams)) {
     // OK, `.` identifier.
   } else if (context.ConsumeAndAddLeafNodeIf(Lex::TokenKind::Base,
                                              NodeKind::BaseName)) {
@@ -43,11 +44,11 @@ static auto HandlePeriodOrArrow(Context& context, NodeKind node_kind,
     // If we see a keyword, assume it was intended to be a name.
     // TODO: Should keywords be valid here?
     if (context.PositionKind().is_keyword()) {
-      context.AddLeafNode(NodeKind::IdentifierName, context.Consume(),
-                          /*has_error=*/true);
+      context.AddLeafNode(NodeKind::IdentifierNameNotBeforeParams,
+                          context.Consume(), /*has_error=*/true);
     } else {
-      context.AddLeafNode(NodeKind::IdentifierName, *context.position(),
-                          /*has_error=*/true);
+      context.AddLeafNode(NodeKind::IdentifierNameNotBeforeParams,
+                          *context.position(), /*has_error=*/true);
       // Indicate the error to the parent state so that it can avoid producing
       // more errors.
       context.ReturnErrorOnState();

diff --git a/toolchain/parse/handle_requirement.cpp b/toolchain/parse/handle_requirement.cpp
@@ -18,7 +18,7 @@ auto HandleRequirementBegin(Context& context) -> void {
       context.PositionKind(static_cast<Lookahead>(2)) ==
           Lex::TokenKind::Equal) {
     auto period = context.Consume();
-    context.AddNode(NodeKind::IdentifierName, context.Consume(),
+    context.AddNode(NodeKind::IdentifierNameNotBeforeParams, context.Consume(),
                     /*has_error=*/false);
     context.AddNode(NodeKind::DesignatorExpr, period, /*has_error=*/false);
     state.token = context.Consume();