From 19532967fa4bec0d72dc38f087c0f5e25c0ca306 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Thu, 17 Apr 2025 14:58:37 -0700 Subject: [PATCH] Stop pushing a fake generic for the duration of check. (#5326) This fake generic was used for two reasons: - The declaration name stack assumes that each declaration name is processed within a generic scope. This is important if the name might have generic parameters, which are always parsed even for declarations that disallow them in check. - Out-of-line redeclarations of generic entities produce instructions with symbolic constant values in non-generic scopes. The former case is addressed by pushing a generic each time we start a declaration name, even if we will reject generic parameters later. The latter case is worked around for now by not building a symbolic constant type or value for instructions that appear outside of any generic, and will be addressed more completely by #5310 and follow-ups. --- toolchain/check/check_unit.cpp | 7 ------- toolchain/check/generic_region_stack.cpp | 15 +++++++++++++++ toolchain/check/handle_alias.cpp | 7 +++++++ toolchain/check/handle_choice.cpp | 10 +++++----- toolchain/check/handle_class.cpp | 4 ++-- toolchain/check/handle_export.cpp | 5 +++++ toolchain/check/handle_function.cpp | 4 ++-- toolchain/check/handle_impl.cpp | 6 +++--- toolchain/check/handle_interface.cpp | 4 ++-- toolchain/check/handle_namespace.cpp | 6 ++++++ 10 files changed, 47 insertions(+), 21 deletions(-) diff --git a/toolchain/check/check_unit.cpp b/toolchain/check/check_unit.cpp index 62fd5595958f..f4a805ff2b99 100644 --- a/toolchain/check/check_unit.cpp +++ b/toolchain/check/check_unit.cpp @@ -68,10 +68,6 @@ auto CheckUnit::Run() -> void { // Add a block for the file. context_.inst_block_stack().Push(); - // TODO: Remove this and the pop in `FinishRun` once we properly push and pop - // in the right places. - context_.generic_region_stack().Push(); - InitPackageScopeAndImports(); // Eagerly import the impls declared in the api file to prepare to redeclare @@ -509,9 +505,6 @@ auto CheckUnit::CheckRequiredDefinitions() -> void { } auto CheckUnit::FinishRun() -> void { - // TODO: Remove this once we properly push and pop in the right places. - context_.generic_region_stack().Pop(); - CheckRequiredDeclarations(); CheckRequiredDefinitions(); diff --git a/toolchain/check/generic_region_stack.cpp b/toolchain/check/generic_region_stack.cpp index c73b7a5b79e5..700e5bd02da8 100644 --- a/toolchain/check/generic_region_stack.cpp +++ b/toolchain/check/generic_region_stack.cpp @@ -11,6 +11,21 @@ auto GenericRegionStack::Push() -> void { dependent_insts_stack_.PushArray(); } auto GenericRegionStack::Pop() -> void { dependent_insts_stack_.PopArray(); } auto GenericRegionStack::AddDependentInst(DependentInst inst) -> void { + if (dependent_insts_stack_.empty()) { + // If we don't have a generic region here, leave the dependent instruction + // unattached. This happens for out-of-line redeclarations of members of + // dependent scopes: + // + // class A(T:! type) { + // fn F(); + // } + // // Has generic type and constant value, but no generic region. + // fn A(T:! type).F() {} + // + // TODO: Use a different instruction kind for out-of-line definitions and + // CHECK this doesn't happen. + return; + } dependent_insts_stack_.AppendToTop(inst); } diff --git a/toolchain/check/handle_alias.cpp b/toolchain/check/handle_alias.cpp index 994fea53f3cf..b13f3ccdfb84 100644 --- a/toolchain/check/handle_alias.cpp +++ b/toolchain/check/handle_alias.cpp @@ -3,6 +3,7 @@ // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception #include "toolchain/check/context.h" +#include "toolchain/check/generic.h" #include "toolchain/check/handle.h" #include "toolchain/check/inst.h" #include "toolchain/check/modifiers.h" @@ -15,6 +16,10 @@ namespace Carbon::Check { auto HandleParseNode(Context& context, Parse::AliasIntroducerId /*node_id*/) -> bool { + // Aliases can't be generic, but we might have parsed a generic parameter in + // their name, so enter a generic scope just in case. + StartGenericDecl(context); + // Optional modifiers and the name follow. context.decl_introducer_state_stack().Push(); context.decl_name_stack().PushScopeAndStartName(); return true; @@ -31,6 +36,8 @@ auto HandleParseNode(Context& context, Parse::AliasId /*node_id*/) -> bool { auto name_context = context.decl_name_stack().FinishName( PopNameComponentWithoutParams(context, Lex::TokenKind::Alias)); + DiscardGenericDecl(context); + auto introducer = context.decl_introducer_state_stack().Pop(); LimitModifiersOnDecl(context, introducer, KeywordModifierSet::Access); diff --git a/toolchain/check/handle_choice.cpp b/toolchain/check/handle_choice.cpp index 1bc006b7988c..1b6a767c4d52 100644 --- a/toolchain/check/handle_choice.cpp +++ b/toolchain/check/handle_choice.cpp @@ -23,18 +23,18 @@ namespace Carbon::Check { auto HandleParseNode(Context& context, Parse::ChoiceIntroducerId node_id) -> bool { + // This choice is potentially generic. + StartGenericDecl(context); // Create an instruction block to hold the instructions created as part of the // choice signature, such as generic parameters. context.inst_block_stack().Push(); + // There's no modifiers on a choice, but this informs how to typecheck any + // generic binding pattern. + context.decl_introducer_state_stack().Push(); // Push the bracketing node. context.node_stack().Push(node_id); // The choice's name follows. context.decl_name_stack().PushScopeAndStartName(); - // There's no modifiers on a choice, but this informs how to typecheck any - // generic binding pattern. - context.decl_introducer_state_stack().Push(); - // This choice is potentially generic. - StartGenericDefinition(context); return true; } diff --git a/toolchain/check/handle_class.cpp b/toolchain/check/handle_class.cpp index c846595a2201..09e443780238 100644 --- a/toolchain/check/handle_class.cpp +++ b/toolchain/check/handle_class.cpp @@ -33,6 +33,8 @@ namespace Carbon::Check { auto HandleParseNode(Context& context, Parse::ClassIntroducerId node_id) -> bool { + // This class is potentially generic. + StartGenericDecl(context); // Create an instruction block to hold the instructions created as part of the // class signature, such as generic parameters. context.inst_block_stack().Push(); @@ -41,8 +43,6 @@ auto HandleParseNode(Context& context, Parse::ClassIntroducerId node_id) // Optional modifiers and the name follow. context.decl_introducer_state_stack().Push(); context.decl_name_stack().PushScopeAndStartName(); - // This class is potentially generic. - StartGenericDecl(context); return true; } diff --git a/toolchain/check/handle_export.cpp b/toolchain/check/handle_export.cpp index 7731472aad82..411be49370fe 100644 --- a/toolchain/check/handle_export.cpp +++ b/toolchain/check/handle_export.cpp @@ -4,6 +4,7 @@ #include "toolchain/check/context.h" #include "toolchain/check/decl_name_stack.h" +#include "toolchain/check/generic.h" #include "toolchain/check/handle.h" #include "toolchain/check/inst.h" #include "toolchain/check/modifiers.h" @@ -17,6 +18,9 @@ namespace Carbon::Check { auto HandleParseNode(Context& context, Parse::ExportIntroducerId /*node_id*/) -> bool { + // Export declarations can't be generic, but we might have parsed a generic + // parameter in their name, so enter a generic scope just in case. + StartGenericDecl(context); context.decl_introducer_state_stack().Push(); // TODO: Probably need to update DeclNameStack to restrict to only namespaces. context.decl_name_stack().PushScopeAndStartName(); @@ -26,6 +30,7 @@ auto HandleParseNode(Context& context, Parse::ExportIntroducerId /*node_id*/) auto HandleParseNode(Context& context, Parse::ExportDeclId node_id) -> bool { auto name_context = context.decl_name_stack().FinishName( PopNameComponentWithoutParams(context, Lex::TokenKind::Export)); + DiscardGenericDecl(context); context.decl_name_stack().PopScope(); auto introducer = diff --git a/toolchain/check/handle_function.cpp b/toolchain/check/handle_function.cpp index caeaf33a4d67..4706a4bac688 100644 --- a/toolchain/check/handle_function.cpp +++ b/toolchain/check/handle_function.cpp @@ -40,6 +40,8 @@ namespace Carbon::Check { auto HandleParseNode(Context& context, Parse::FunctionIntroducerId node_id) -> bool { + // The function is potentially generic. + StartGenericDecl(context); // Create an instruction block to hold the instructions created as part of the // function signature, such as parameter and return types. context.inst_block_stack().Push(); @@ -48,8 +50,6 @@ auto HandleParseNode(Context& context, Parse::FunctionIntroducerId node_id) // Optional modifiers and the name follow. context.decl_introducer_state_stack().Push(); context.decl_name_stack().PushScopeAndStartName(); - // The function is potentially generic. - StartGenericDecl(context); return true; } diff --git a/toolchain/check/handle_impl.cpp b/toolchain/check/handle_impl.cpp index c98006cd32ef..22abc64aa9aa 100644 --- a/toolchain/check/handle_impl.cpp +++ b/toolchain/check/handle_impl.cpp @@ -28,6 +28,9 @@ namespace Carbon::Check { auto HandleParseNode(Context& context, Parse::ImplIntroducerId node_id) -> bool { + // This might be a generic impl. + StartGenericDecl(context); + // Create an instruction block to hold the instructions created for the type // and interface. context.inst_block_stack().Push(); @@ -42,9 +45,6 @@ auto HandleParseNode(Context& context, Parse::ImplIntroducerId node_id) // consistent to imagine that it does. This also gives us a scope for implicit // parameters. context.decl_name_stack().PushScopeAndStartName(); - - // This might be a generic impl. - StartGenericDecl(context); return true; } diff --git a/toolchain/check/handle_interface.cpp b/toolchain/check/handle_interface.cpp index b349ac17e59e..c1dcd7f4d185 100644 --- a/toolchain/check/handle_interface.cpp +++ b/toolchain/check/handle_interface.cpp @@ -21,6 +21,8 @@ namespace Carbon::Check { auto HandleParseNode(Context& context, Parse::InterfaceIntroducerId node_id) -> bool { + // This interface is potentially generic. + StartGenericDecl(context); // Create an instruction block to hold the instructions created as part of the // interface signature, such as generic parameters. context.inst_block_stack().Push(); @@ -29,8 +31,6 @@ auto HandleParseNode(Context& context, Parse::InterfaceIntroducerId node_id) // Optional modifiers and the name follow. context.decl_introducer_state_stack().Push(); context.decl_name_stack().PushScopeAndStartName(); - // This interface is potentially generic. - StartGenericDecl(context); return true; } diff --git a/toolchain/check/handle_namespace.cpp b/toolchain/check/handle_namespace.cpp index 0be9744a1599..630de7f0fd66 100644 --- a/toolchain/check/handle_namespace.cpp +++ b/toolchain/check/handle_namespace.cpp @@ -4,6 +4,7 @@ #include "toolchain/check/context.h" #include "toolchain/check/decl_introducer_state.h" +#include "toolchain/check/generic.h" #include "toolchain/check/handle.h" #include "toolchain/check/inst.h" #include "toolchain/check/modifiers.h" @@ -19,6 +20,9 @@ namespace Carbon::Check { auto HandleParseNode(Context& context, Parse::NamespaceStartId /*node_id*/) -> bool { + // Namespaces can't be generic, but we might have parsed a generic parameter + // in their name, so enter a generic scope just in case. + StartGenericDecl(context); // Optional modifiers and the name follow. context.decl_introducer_state_stack().Push(); context.decl_name_stack().PushScopeAndStartName(); @@ -35,6 +39,8 @@ auto HandleParseNode(Context& context, Parse::NamespaceId node_id) -> bool { auto name_context = context.decl_name_stack().FinishName( PopNameComponentWithoutParams(context, Lex::TokenKind::Namespace)); + DiscardGenericDecl(context); + auto introducer = context.decl_introducer_state_stack().Pop(); LimitModifiersOnDecl(context, introducer, KeywordModifierSet::None);