From 43065a1257b2cca121ad9b28ee29c0b55a755fdd Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Fri, 14 Jul 2023 16:16:16 -0700 Subject: [PATCH] Finish refactoring Push/Pop for stronger type handling. (#2987) This adds a distinction between Unused and SoloParseNode, rather than equating the two. This is intended to help identify nodes which are getting pushed but maybe don't need to be. Not totally done because I want to adjust declaration name handling due to a quirk with how it mixes Name with Expression, but almost done. Once that's done the type punning will be completely gone. --- toolchain/semantics/semantics_context.cpp | 4 +- .../semantics_handle_call_expression.cpp | 2 +- .../semantics/semantics_handle_function.cpp | 2 +- .../semantics_handle_if_expression.cpp | 6 +- .../semantics_handle_if_statement.cpp | 2 +- toolchain/semantics/semantics_handle_name.cpp | 6 +- .../semantics/semantics_handle_operator.cpp | 8 +- .../semantics/semantics_handle_paren.cpp | 2 +- .../semantics_handle_pattern_binding.cpp | 2 +- .../semantics/semantics_handle_statement.cpp | 4 +- .../semantics/semantics_handle_struct.cpp | 5 +- .../semantics/semantics_handle_variable.cpp | 2 +- toolchain/semantics/semantics_node_stack.h | 181 ++++++++++-------- 13 files changed, 118 insertions(+), 108 deletions(-) diff --git a/toolchain/semantics/semantics_context.cpp b/toolchain/semantics/semantics_context.cpp index 25a7da23a132e..95638d13f89d4 100644 --- a/toolchain/semantics/semantics_context.cpp +++ b/toolchain/semantics/semantics_context.cpp @@ -312,7 +312,7 @@ auto SemanticsContext::PopDeclarationName() -> DeclarationNameContext { } else { // The name had no qualifiers, so we need to process the node now. auto [parse_node, node_or_name_id] = - node_stack_.PopWithParseNode(); + node_stack_.PopExpressionWithParseNode(); ApplyDeclarationNameQualifier(parse_node, node_or_name_id); } @@ -547,7 +547,7 @@ auto SemanticsContext::ParamOrArgSave(bool for_args) -> void { // For an argument, we add a stub reference to the expression on the top of // the stack. There may not be anything on the IR prior to this. auto [entry_parse_node, entry_node_id] = - node_stack_.PopWithParseNode(); + node_stack_.PopExpressionWithParseNode(); param_or_arg_id = AddNode(SemanticsNode::StubReference::Make( entry_parse_node, semantics_ir_->GetNode(entry_node_id).type_id(), entry_node_id)); diff --git a/toolchain/semantics/semantics_handle_call_expression.cpp b/toolchain/semantics/semantics_handle_call_expression.cpp index b1bb43844f588..b6b9977cbbb4e 100644 --- a/toolchain/semantics/semantics_handle_call_expression.cpp +++ b/toolchain/semantics/semantics_handle_call_expression.cpp @@ -57,7 +57,7 @@ auto SemanticsHandleCallExpressionComma(SemanticsContext& context, auto SemanticsHandleCallExpressionStart(SemanticsContext& context, ParseTree::Node parse_node) -> bool { - auto name_id = context.node_stack().Pop(); + auto name_id = context.node_stack().PopExpression(); context.node_stack().Push(parse_node, name_id); context.ParamOrArgStart(); return true; diff --git a/toolchain/semantics/semantics_handle_function.cpp b/toolchain/semantics/semantics_handle_function.cpp index bacb205f0baa7..b776aebdd3ff7 100644 --- a/toolchain/semantics/semantics_handle_function.cpp +++ b/toolchain/semantics/semantics_handle_function.cpp @@ -111,7 +111,7 @@ auto SemanticsHandleReturnType(SemanticsContext& context, ParseTree::Node parse_node) -> bool { // Propagate the type expression. auto [type_parse_node, type_node_id] = - context.node_stack().PopWithParseNode(); + context.node_stack().PopExpressionWithParseNode(); auto cast_node_id = context.ExpressionAsType(type_parse_node, type_node_id); context.node_stack().Push(parse_node, cast_node_id); return true; diff --git a/toolchain/semantics/semantics_handle_if_expression.cpp b/toolchain/semantics/semantics_handle_if_expression.cpp index 521bd1f5638b1..5c9ae5cbd928d 100644 --- a/toolchain/semantics/semantics_handle_if_expression.cpp +++ b/toolchain/semantics/semantics_handle_if_expression.cpp @@ -8,7 +8,7 @@ namespace Carbon { auto SemanticsHandleIfExpressionIf(SemanticsContext& context, ParseTree::Node if_node) -> bool { - auto cond_value_id = context.node_stack().Pop(); + auto cond_value_id = context.node_stack().PopExpression(); context.node_stack().Push(if_node); @@ -35,10 +35,10 @@ auto SemanticsHandleIfExpressionThen(SemanticsContext& context, auto SemanticsHandleIfExpressionElse(SemanticsContext& context, ParseTree::Node else_node) -> bool { - auto else_value_id = context.node_stack().Pop(); + auto else_value_id = context.node_stack().PopExpression(); auto [then_node, then_end_block_id] = context.node_stack().PopWithParseNode(); - auto then_value_id = context.node_stack().Pop(); + auto then_value_id = context.node_stack().PopExpression(); auto if_node = context.node_stack().PopForSoloParseNode(); diff --git a/toolchain/semantics/semantics_handle_if_statement.cpp b/toolchain/semantics/semantics_handle_if_statement.cpp index 2a300c2cd78e2..a144d0e6ca1ec 100644 --- a/toolchain/semantics/semantics_handle_if_statement.cpp +++ b/toolchain/semantics/semantics_handle_if_statement.cpp @@ -15,7 +15,7 @@ auto SemanticsHandleIfConditionStart(SemanticsContext& /*context*/, auto SemanticsHandleIfCondition(SemanticsContext& context, ParseTree::Node parse_node) -> bool { // Convert the condition to `bool`. - auto cond_value_id = context.node_stack().Pop(); + auto cond_value_id = context.node_stack().PopExpression(); cond_value_id = context.ImplicitAsBool(parse_node, cond_value_id); // Create the then block and the else block, and branch to the right one. If diff --git a/toolchain/semantics/semantics_handle_name.cpp b/toolchain/semantics/semantics_handle_name.cpp index ccbee0aedcb5f..df38af325249e 100644 --- a/toolchain/semantics/semantics_handle_name.cpp +++ b/toolchain/semantics/semantics_handle_name.cpp @@ -11,7 +11,7 @@ auto SemanticsHandleMemberAccessExpression(SemanticsContext& context, ParseTree::Node parse_node) -> bool { SemanticsStringId name_id = context.node_stack().Pop(); - auto base_id = context.node_stack().Pop(); + auto base_id = context.node_stack().PopExpression(); auto base = context.semantics_ir().GetNode(base_id); if (base.kind() == SemanticsNodeKind::Namespace) { // For a namespace, just resolve the name. @@ -92,12 +92,12 @@ auto SemanticsHandleQualifiedDeclaration(SemanticsContext& context, // QualifiedDeclaration as the first child, and an Identifier or expression as // the second child. auto [parse_node2, node_or_name_id2] = - context.node_stack().PopWithParseNode(); + context.node_stack().PopExpressionWithParseNode(); if (context.parse_tree().node_kind(context.node_stack().PeekParseNode()) != ParseNodeKind::QualifiedDeclaration) { // First QualifiedDeclaration in a chain. auto [parse_node1, node_or_name_id1] = - context.node_stack().PopWithParseNode(); + context.node_stack().PopExpressionWithParseNode(); context.ApplyDeclarationNameQualifier(parse_node1, node_or_name_id1); // Add the QualifiedDeclaration so that it can be used for bracketing. context.node_stack().Push(parse_node); diff --git a/toolchain/semantics/semantics_handle_operator.cpp b/toolchain/semantics/semantics_handle_operator.cpp index 693e7a6d2cba3..5437c3d7d72a0 100644 --- a/toolchain/semantics/semantics_handle_operator.cpp +++ b/toolchain/semantics/semantics_handle_operator.cpp @@ -8,8 +8,8 @@ namespace Carbon { auto SemanticsHandleInfixOperator(SemanticsContext& context, ParseTree::Node parse_node) -> bool { - auto rhs_id = context.node_stack().Pop(); - auto lhs_id = context.node_stack().Pop(); + auto rhs_id = context.node_stack().PopExpression(); + auto lhs_id = context.node_stack().PopExpression(); // Figure out the operator for the token. auto token = context.parse_tree().node_token(parse_node); @@ -66,7 +66,7 @@ auto SemanticsHandlePostfixOperator(SemanticsContext& context, auto SemanticsHandlePrefixOperator(SemanticsContext& context, ParseTree::Node parse_node) -> bool { - auto value_id = context.node_stack().Pop(); + auto value_id = context.node_stack().PopExpression(); // Figure out the operator for the token. auto token = context.parse_tree().node_token(parse_node); @@ -90,7 +90,7 @@ auto SemanticsHandlePrefixOperator(SemanticsContext& context, auto SemanticsHandleShortCircuitOperand(SemanticsContext& context, ParseTree::Node parse_node) -> bool { // Convert the condition to `bool`. - auto cond_value_id = context.node_stack().Pop(); + auto cond_value_id = context.node_stack().PopExpression(); cond_value_id = context.ImplicitAsBool(parse_node, cond_value_id); auto bool_type_id = context.semantics_ir().GetNode(cond_value_id).type_id(); diff --git a/toolchain/semantics/semantics_handle_paren.cpp b/toolchain/semantics/semantics_handle_paren.cpp index 602fe46d3afea..5d69d60e54961 100644 --- a/toolchain/semantics/semantics_handle_paren.cpp +++ b/toolchain/semantics/semantics_handle_paren.cpp @@ -8,7 +8,7 @@ namespace Carbon { auto SemanticsHandleParenExpression(SemanticsContext& context, ParseTree::Node parse_node) -> bool { - auto value_id = context.node_stack().Pop(); + auto value_id = context.node_stack().PopExpression(); context.node_stack() .PopAndDiscardSoloParseNode< ParseNodeKind::ParenExpressionOrTupleLiteralStart>(); diff --git a/toolchain/semantics/semantics_handle_pattern_binding.cpp b/toolchain/semantics/semantics_handle_pattern_binding.cpp index e728455534820..7e7350f251806 100644 --- a/toolchain/semantics/semantics_handle_pattern_binding.cpp +++ b/toolchain/semantics/semantics_handle_pattern_binding.cpp @@ -20,7 +20,7 @@ auto SemanticsHandleGenericPatternBinding(SemanticsContext& context, auto SemanticsHandlePatternBinding(SemanticsContext& context, ParseTree::Node parse_node) -> bool { auto [type_node, parsed_type_id] = - context.node_stack().PopWithParseNode(); + context.node_stack().PopExpressionWithParseNode(); auto cast_type_id = context.ExpressionAsType(type_node, parsed_type_id); // Get the name. diff --git a/toolchain/semantics/semantics_handle_statement.cpp b/toolchain/semantics/semantics_handle_statement.cpp index c84c58353db22..e347aba2b12c3 100644 --- a/toolchain/semantics/semantics_handle_statement.cpp +++ b/toolchain/semantics/semantics_handle_statement.cpp @@ -13,7 +13,7 @@ auto SemanticsHandleExpressionStatement(SemanticsContext& context, // Pop the expression without investigating its contents. // TODO: This will probably eventually need to do some "do not discard" // analysis. - context.node_stack().PopAndDiscardId(); + context.node_stack().PopExpression(); return true; } @@ -42,7 +42,7 @@ auto SemanticsHandleReturnStatement(SemanticsContext& context, context.AddNode(SemanticsNode::Return::Make(parse_node)); } else { - auto arg = context.node_stack().Pop(); + auto arg = context.node_stack().PopExpression(); context.node_stack() .PopAndDiscardSoloParseNode(); diff --git a/toolchain/semantics/semantics_handle_struct.cpp b/toolchain/semantics/semantics_handle_struct.cpp index ea08616a9787f..bc5a9ec373752 100644 --- a/toolchain/semantics/semantics_handle_struct.cpp +++ b/toolchain/semantics/semantics_handle_struct.cpp @@ -27,8 +27,7 @@ auto SemanticsHandleStructFieldDesignator(SemanticsContext& context, auto SemanticsHandleStructFieldType(SemanticsContext& context, ParseTree::Node parse_node) -> bool { - auto [type_node, type_id] = - context.node_stack().PopWithParseNode(); + auto [type_node, type_id] = context.node_stack().PopExpressionWithParseNode(); SemanticsTypeId cast_type_id = context.ExpressionAsType(type_node, type_id); auto [name_node, name_id] = @@ -48,7 +47,7 @@ auto SemanticsHandleStructFieldUnknown(SemanticsContext& context, auto SemanticsHandleStructFieldValue(SemanticsContext& context, ParseTree::Node parse_node) -> bool { auto [value_parse_node, value_node_id] = - context.node_stack().PopWithParseNode(); + context.node_stack().PopExpressionWithParseNode(); SemanticsStringId name_id = context.node_stack().Pop(); // Store the name for the type. diff --git a/toolchain/semantics/semantics_handle_variable.cpp b/toolchain/semantics/semantics_handle_variable.cpp index 4d380a2403bcc..77c3bde94d7ed 100644 --- a/toolchain/semantics/semantics_handle_variable.cpp +++ b/toolchain/semantics/semantics_handle_variable.cpp @@ -15,7 +15,7 @@ auto SemanticsHandleVariableDeclaration(SemanticsContext& context, context.parse_tree().node_kind(context.node_stack().PeekParseNode()) != ParseNodeKind::PatternBinding; if (has_init) { - expr_node_id = context.node_stack().Pop(); + expr_node_id = context.node_stack().PopExpression(); context.node_stack() .PopAndDiscardSoloParseNode(); } diff --git a/toolchain/semantics/semantics_node_stack.h b/toolchain/semantics/semantics_node_stack.h index 1995efceebd4b..bce4fca2c0052 100644 --- a/toolchain/semantics/semantics_node_stack.h +++ b/toolchain/semantics/semantics_node_stack.h @@ -38,7 +38,7 @@ class SemanticsNodeStack { // IR generated by the node. auto Push(ParseTree::Node parse_node) -> void { CARBON_CHECK(ParseNodeKindToIdKind(parse_tree_->node_kind(parse_node)) == - IdKind::Unused) + IdKind::SoloParseNode) << "Parse kind expects an Id: " << parse_tree_->node_kind(parse_node); CARBON_VLOG() << "Node Push " << stack_.size() << ": " << parse_tree_->node_kind(parse_node) << " -> \n"; @@ -67,100 +67,102 @@ class SemanticsNodeStack { auto PopAndIgnore() -> void { PopEntry(); } // Pops the top of the stack and returns the parse_node. + template auto PopForSoloParseNode() -> ParseTree::Node { Entry back = PopEntry(); - RequireSoloParseNode(back); + RequireIdKind(ParseNodeKind::Create(RequiredParseKind), + IdKind::SoloParseNode); + RequireParseKind(back.parse_node); return back.parse_node; } - // Pops the top of the stack and returns the parse_node. - template - auto PopForSoloParseNode() -> ParseTree::Node { - auto parse_node = PopForSoloParseNode(); - RequireParseKind(parse_node, ParseNodeKind::Create(PopParseKind)); - return parse_node; - } - // Pops the top of the stack. - template + template auto PopAndDiscardSoloParseNode() -> void { - PopForSoloParseNode(); + PopForSoloParseNode(); } // Pops the top of the stack and returns the parse_node and the ID. - template - auto PopWithParseNode() -> std::pair { - Entry back = PopEntry(); - RequireValidId(back); - return {back.parse_node, back.id()}; + auto PopExpressionWithParseNode() + -> std::pair { + return PopWithParseNode(); } // Pops the top of the stack and returns the parse_node and the ID. - template + template auto PopWithParseNode() -> auto { - if constexpr (ParseNodeKindToIdKind(ParseNodeKind::Create(PopParseKind)) == - IdKind::SemanticsNodeId) { + constexpr IdKind RequiredIdKind = + ParseNodeKindToIdKind(ParseNodeKind::Create(RequiredParseKind)); + if constexpr (RequiredIdKind == IdKind::SemanticsNodeId) { auto back = PopWithParseNode(); - RequireParseKind(back.first, ParseNodeKind::Create(PopParseKind)); + RequireParseKind(back.first); return back; } - if constexpr (ParseNodeKindToIdKind(ParseNodeKind::Create(PopParseKind)) == - IdKind::SemanticsNodeBlockId) { + if constexpr (RequiredIdKind == IdKind::SemanticsNodeBlockId) { auto back = PopWithParseNode(); - RequireParseKind(back.first, ParseNodeKind::Create(PopParseKind)); + RequireParseKind(back.first); return back; } - if constexpr (ParseNodeKindToIdKind(ParseNodeKind::Create(PopParseKind)) == - IdKind::SemanticsFunctionId) { + if constexpr (RequiredIdKind == IdKind::SemanticsFunctionId) { auto back = PopWithParseNode(); - RequireParseKind(back.first, ParseNodeKind::Create(PopParseKind)); + RequireParseKind(back.first); return back; } - if constexpr (ParseNodeKindToIdKind(ParseNodeKind::Create(PopParseKind)) == - IdKind::SemanticsStringId) { + if constexpr (RequiredIdKind == IdKind::SemanticsStringId) { auto back = PopWithParseNode(); - RequireParseKind(back.first, ParseNodeKind::Create(PopParseKind)); + RequireParseKind(back.first); return back; } - if constexpr (ParseNodeKindToIdKind(ParseNodeKind::Create(PopParseKind)) == - IdKind::SemanticsTypeId) { + if constexpr (RequiredIdKind == IdKind::SemanticsTypeId) { auto back = PopWithParseNode(); - RequireParseKind(back.first, ParseNodeKind::Create(PopParseKind)); + RequireParseKind(back.first); return back; } + CARBON_FATAL() << "Unpoppable IdKind for parse kind: " + << ParseNodeKind::Create(RequiredParseKind) + << "; see value in ParseNodeKindToIdKind"; } - // Pops the top of the stack and returns the ID. - template - auto Pop() -> IdT { - return PopWithParseNode().second; + // Pops an expression from the top of the stack and returns the ID. + // Expressions map multiple ParseNodeKinds to SemanticsNodeId always. + auto PopExpression() -> SemanticsNodeId { + return PopExpressionWithParseNode().second; } // Pops the top of the stack and returns the ID. - template + template auto Pop() -> auto { - return PopWithParseNode().second; - } - - // Pops the top of the stack, and discards the ID. - auto PopAndDiscardId() -> void { PopWithParseNode(); } - - // Pops the top of the stack, and discards the ID. - template - auto PopAndDiscardId() -> void { - PopWithParseNode(); + return PopWithParseNode().second; } // Peeks at the parse_node of the top of the stack. auto PeekParseNode() -> ParseTree::Node { return stack_.back().parse_node; } // Peeks at the ID of the top of the stack. - template - auto Peek(ParseNodeKind parse_kind) -> IdT { + template + auto Peek() -> auto { Entry back = stack_.back(); - RequireParseKind(back.parse_node, parse_kind); - RequireValidId(back); - return back.id(); + RequireParseKind(back.parse_node); + constexpr IdKind RequiredIdKind = + ParseNodeKindToIdKind(ParseNodeKind::Create(RequiredParseKind)); + if constexpr (RequiredIdKind == IdKind::SemanticsNodeId) { + return back.id(); + } + if constexpr (RequiredIdKind == IdKind::SemanticsNodeBlockId) { + return back.id(); + } + if constexpr (RequiredIdKind == IdKind::SemanticsFunctionId) { + return back.id(); + } + if constexpr (RequiredIdKind == IdKind::SemanticsStringId) { + return back.id(); + } + if constexpr (RequiredIdKind == IdKind::SemanticsTypeId) { + return back.id(); + } + CARBON_FATAL() << "Unpeekable IdKind for parse kind: " + << ParseNodeKind::Create(RequiredParseKind) + << "; see value in ParseNodeKindToIdKind"; } // Prints the stack for a stack dump. @@ -170,12 +172,16 @@ class SemanticsNodeStack { auto size() const -> size_t { return stack_.size(); } private: + // Possible associated ID types. enum class IdKind { SemanticsNodeId, SemanticsNodeBlockId, SemanticsFunctionId, SemanticsStringId, SemanticsTypeId, + // No associated ID type. + SoloParseNode, + // Not expected in the node stack. Unused, }; @@ -228,21 +234,6 @@ class SemanticsNodeStack { SemanticsStringId name_id; SemanticsTypeId type_id; }; - - // APIs rely on type punning. They read node_id.is_valid, even though that - // may not be the active union member. These asserts enforce standard layout - // in order to help ensure that works. - // TODO: Use is_layout_compatible in C++20. - static_assert(std::is_standard_layout_v, - "Need standard layout for type punning"); - static_assert(std::is_standard_layout_v, - "Need standard layout for type punning"); - static_assert(std::is_standard_layout_v, - "Need standard layout for type punning"); - static_assert(std::is_standard_layout_v, - "Need standard layout for type punning"); - static_assert(std::is_standard_layout_v, - "Need standard layout for type punning"); }; static_assert(sizeof(Entry) == 8, "Unexpected Entry size"); @@ -274,6 +265,19 @@ class SemanticsNodeStack { return IdKind::SemanticsStringId; case Carbon::ParseNodeKind::ReturnType: return IdKind::SemanticsTypeId; + case Carbon::ParseNodeKind::CodeBlockStart: + case Carbon::ParseNodeKind::FunctionIntroducer: + case Carbon::ParseNodeKind::IfCondition: + case Carbon::ParseNodeKind::IfExpressionIf: + case Carbon::ParseNodeKind::ParameterListStart: + case Carbon::ParseNodeKind::ParenExpressionOrTupleLiteralStart: + case Carbon::ParseNodeKind::QualifiedDeclaration: + case Carbon::ParseNodeKind::ReturnStatementStart: + case Carbon::ParseNodeKind::StructFieldType: + case Carbon::ParseNodeKind::StructLiteralOrStructTypeLiteralStart: + case Carbon::ParseNodeKind::VariableInitializer: + case Carbon::ParseNodeKind::VariableIntroducer: + return IdKind::SoloParseNode; default: return IdKind::Unused; } @@ -310,27 +314,34 @@ class SemanticsNodeStack { return back; } - // Require an entry to have the given ParseNodeKind. - auto RequireParseKind(ParseTree::Node parse_node, ParseNodeKind require_kind) - -> void { - auto actual_kind = parse_tree_->node_kind(parse_node); - CARBON_CHECK(require_kind == actual_kind) - << "Expected " << require_kind << ", found " << actual_kind; + // Pops the top of the stack and returns the parse_node and the ID. + template + auto PopWithParseNode() -> std::pair { + Entry back = PopEntry(); + RequireIdKind(parse_tree_->node_kind(back.parse_node), + IdTypeToIdKind()); + return {back.parse_node, back.id()}; } - // Requires an entry to have a invalid node_id. - auto RequireSoloParseNode(Entry entry) -> void { - // See above comment on type punning. - CARBON_CHECK(!entry.node_id.is_valid()) - << "Expected invalid id on " << parse_tree_->node_kind(entry.parse_node) - << ", was " << entry.node_id << " (may not be node)"; + // Require a ParseNodeKind be mapped to a particular IdKind. + auto RequireIdKind(ParseNodeKind parse_kind, IdKind id_kind) -> void { + // TODO: Name can be popped as a node_id by declaration name handling. Will + // refactor to remove this quirk. + if (parse_kind == ParseNodeKind::Name && + id_kind == IdKind::SemanticsNodeId) { + return; + } + CARBON_CHECK(ParseNodeKindToIdKind(parse_kind) == id_kind) + << "Unexpected IdKind mapping for " << parse_kind; } - // Requires an entry to have a valid id. - auto RequireValidId(Entry entry) -> void { - // See above comment on type punning. - CARBON_CHECK(entry.node_id.is_valid()) - << "Expected valid id on " << parse_tree_->node_kind(entry.parse_node); + // Require an entry to have the given ParseNodeKind. + template + auto RequireParseKind(ParseTree::Node parse_node) -> void { + auto actual_kind = parse_tree_->node_kind(parse_node); + CARBON_CHECK(RequiredParseKind == actual_kind) + << "Expected " << ParseNodeKind::Create(RequiredParseKind) << ", found " + << actual_kind; } // The file's parse tree.