Skip to content

Build the binding EntityName in CompileTimeBindingPatternStart #6224

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from
Open

Build the binding EntityName in CompileTimeBindingPatternStart #6224

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cf75142
entity-name-binding
danakj Oct 15, 2025
519158f
review
danakj Oct 16, 2025
3fc16a7
Merge remote-tracking branch 'origin/trunk' into entity-name-binding
danakj Oct 16, 2025
d4991a9
no extra pop
danakj Oct 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions toolchain/check/cpp/import.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1423,13 +1423,15 @@ static auto MakeParamPatternsBlockId(Context& context, SemIR::LocId loc_id,
SemIR::LocId param_loc_id =
AddImportIRInst(context.sem_ir(), param->getLocation());

// TODO: Fix this once templates are supported.
bool is_template = false;
// TODO: Fix this once generics are supported.
bool is_generic = false;
auto entity_name_id = context.entity_names().AddSymbolicBindingName(
name_id, context.scope_stack().PeekNameScopeId(),
// TODO: Fix this once generics are supported.
SemIR::CompileTimeBindIndex::None,
// TODO: Fix this once templates are supported.
/*is_template=*/false);
SemIR::InstId pattern_id =
AddBindingPattern(context, param_loc_id, name_id, type_id,
type_expr_region_id, is_generic, is_template)
AddBindingPatternWithEntityName(context, param_loc_id, entity_name_id,
type_id, type_expr_region_id)
.pattern_id;
pattern_id = AddPatternInst(
context, {param_loc_id,
Expand Down
173 changes: 122 additions & 51 deletions toolchain/check/handle_binding_pattern.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,40 +30,31 @@ auto HandleParseNode(Context& context, Parse::UnderscoreNameId node_id)

// TODO: make this function shorter by factoring pieces out.
static auto HandleAnyBindingPattern(Context& context, Parse::NodeId node_id,
Parse::NodeId name_node,
SemIR::EntityNameId entity_name_id,
Parse::NodeId type_node,
SemIR::InstId parsed_type_inst_id,
Parse::NodeKind node_kind) -> bool {
// TODO: split this into smaller, more focused functions.
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);
ExprAsType(context, type_node, parsed_type_inst_id);

SemIR::ExprRegionId type_expr_region_id =
EndSubpatternAsExpr(context, cast_type_inst_id);

// The name in a template binding may be wrapped in `template`.
bool is_generic = node_kind == Parse::NodeKind::CompileTimeBindingPattern;
auto is_template =
context.node_stack()
.PopAndDiscardSoloNodeIdIf<Parse::NodeKind::TemplateBindingName>();
// A non-generic template binding is diagnosed by the parser.
is_template &= is_generic;

auto [name_node, name_id] = context.node_stack().PopNameWithNodeId();
const auto& entity_name = context.entity_names().Get(entity_name_id);
auto name_id = entity_name.name_id;
CARBON_CHECK((node_kind == Parse::NodeKind::CompileTimeBindingPattern) ==
entity_name.bind_index().has_value());

const DeclIntroducerState& introducer =
context.decl_introducer_state_stack().innermost();

auto make_binding_pattern = [&]() -> SemIR::InstId {
// TODO: Eventually the name will need to support associations with other
// scopes, but right now we don't support qualified names here.
auto binding =
AddBindingPattern(context, name_node, name_id, cast_type_id,
type_expr_region_id, is_generic, is_template);

// TODO: If `is_generic`, then `binding.bind_id is a BindSymbolicName. Subst
// the `.Self` of type `type` in the `cast_type_id` type (a `FacetType`)
// with the `binding.bind_id` itself, and build a new pattern with that.
// This is kind of cyclical. So we need to reuse the EntityNameId, which
// will also reuse the CompileTimeBinding for the new BindSymbolicName.
auto binding = AddBindingPatternWithEntityName(
context, name_node, entity_name_id, cast_type_id, type_expr_region_id);

if (name_id != SemIR::NameId::Underscore) {
// Add name to lookup immediately, so it can be used in the rest of the
Expand Down Expand Up @@ -100,7 +91,8 @@ static auto HandleAnyBindingPattern(Context& context, Parse::NodeId node_id,
case Lex::TokenKind::Fn: {
if (context.full_pattern_stack().CurrentKind() ==
FullPatternStack::Kind::ImplicitParamList &&
!(is_generic || name_id == SemIR::NameId::SelfValue)) {
(node_kind != Parse::NodeKind::CompileTimeBindingPattern &&
name_id != SemIR::NameId::SelfValue)) {
CARBON_DIAGNOSTIC(
ImplictParamMustBeConstant, Error,
"implicit parameters of functions must be constant or `self`");
Expand Down Expand Up @@ -130,7 +122,7 @@ static auto HandleAnyBindingPattern(Context& context, Parse::NodeId node_id,
"`self` parameter only allowed on functions");
context.emitter().Emit(node_id, SelfParameterNotAllowed);
had_error = true;
} else if (!is_generic) {
} else if (node_kind != Parse::NodeKind::CompileTimeBindingPattern) {
CARBON_DIAGNOSTIC(GenericParamMustBeConstant, Error,
"parameters of generic types must be constant");
context.emitter().Emit(node_id, GenericParamMustBeConstant);
Expand All @@ -149,6 +141,12 @@ static auto HandleAnyBindingPattern(Context& context, Parse::NodeId node_id,
// Replace the parameter with `ErrorInst` so that we don't try
// constructing a generic based on it.
result_inst_id = SemIR::ErrorInst::InstId;
if (node_kind == Parse::NodeKind::CompileTimeBindingPattern) {
// Push an error for the EntityName's binding index, since we're not
// constructing an entity with make_binding_pattern().
context.scope_stack().PushCompileTimeBinding(
SemIR::ErrorInst::InstId);
Comment on lines +145 to +148
Copy link
Contributor

@zygoloid zygoloid Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Just musing here, not requesting changes in this PR.]

I wasn't especially happy when I split the numbering of compile-time bindings into separate Add / Push steps, and now the two calls can't be in the same function any more I'm feeling more unhappy about it :) The invariant we're maintaining here is a bit too distributed across the code and a bit too subtle, I think. I wonder if we can rearrange this somehow to make it correct by construction.

Potential idea: could we store a list of EntityNames for compile time bindings on the scope stack instead of a list of instructions? If that works, we could maybe allocate the compile time binding number and the entity name in a single operation.

Copy link
Contributor

@zygoloid zygoloid Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I think this is not just subtle but actually subtly incorrect (but only in a case that we can't yet reach, using a generic lambda). Consider:

fn F(T:! (fn (U:! type) => U)(i32));

I think we will:

  • AddCompileTimeBinding for T -> index 0
  • AddCompileTimeBinding for U -> index 1 (!!)
  • PushCompileTimeBinding for U at index 0 (!!)
  • Build a lambda with one compile-time binding (which incorrectly thinks it's at index 1 but is at index 0)
  • Pop the scope containing U
  • PushCompileTimeBinding for T at index 0

Moving the AddCompileTimeBinding call earlier seems to be what's exposing this issue, but computing the number earlier is necessary for our .Self handling. I think what we should do is:

  • Replace AddCompileTimeBinding with a "peek number of current compile time bindings" call, so we assign the index 0 to both T and U.
  • Make PushCompileTimeBinding assert that the binding's entity name's index matches the next index to be assigned.

}
} else {
result_inst_id = make_binding_pattern();
if (node_kind == Parse::NodeKind::LetBindingPattern) {
Expand Down Expand Up @@ -193,8 +191,6 @@ static auto HandleAnyBindingPattern(Context& context, Parse::NodeId node_id,
}
auto binding_pattern_id = make_binding_pattern();
if (node_kind == Parse::NodeKind::VarBindingPattern) {
CARBON_CHECK(!is_generic);

if (introducer.modifier_set.HasAnyOf(KeywordModifierSet::Returned)) {
// TODO: Should we check this for the `var` as a whole, rather than
// for the name binding?
Expand All @@ -216,23 +212,93 @@ static auto HandleAnyBindingPattern(Context& context, Parse::NodeId node_id,

auto HandleParseNode(Context& context, Parse::LetBindingPatternId node_id)
-> bool {
return HandleAnyBindingPattern(context, node_id,
auto [type_node, type_inst_id] = context.node_stack().PopExprWithNodeId();

// `template` is incorrect here, but is diagnosed in parse.
context.node_stack()
.PopAndDiscardSoloNodeIdIf<Parse::NodeKind::TemplateBindingName>();

auto [name_node, name_id] = context.node_stack().PopNameWithNodeId();
auto entity_name_id = context.entity_names().Add(
{.name_id = name_id,
.parent_scope_id = context.scope_stack().PeekNameScopeId()});

return HandleAnyBindingPattern(context, node_id, name_node, entity_name_id,
type_node, type_inst_id,
Parse::NodeKind::LetBindingPattern);
}

auto HandleParseNode(Context& context, Parse::VarBindingPatternId node_id)
-> bool {
return HandleAnyBindingPattern(context, node_id,
auto [type_node, type_inst_id] = context.node_stack().PopExprWithNodeId();

// `template` is incorrect here, but is diagnosed in parse.
context.node_stack()
.PopAndDiscardSoloNodeIdIf<Parse::NodeKind::TemplateBindingName>();

auto [name_node, name_id] = context.node_stack().PopNameWithNodeId();
auto entity_name_id = context.entity_names().Add(
{.name_id = name_id,
.parent_scope_id = context.scope_stack().PeekNameScopeId()});

return HandleAnyBindingPattern(context, node_id, name_node, entity_name_id,
type_node, type_inst_id,
Parse::NodeKind::VarBindingPattern);
}

auto HandleParseNode(Context& context,
Parse::CompileTimeBindingPatternStartId node_id) -> bool {
// Make a scope to contain the `.Self` facet value for use in the type of the
// compile time binding. This is popped when handling the
// CompileTimeBindingPatternId.
context.scope_stack().PushForSameRegion();
auto is_template =
context.node_stack()
.PopAndDiscardSoloNodeIdIf<Parse::NodeKind::TemplateBindingName>();

auto name_id = context.node_stack().PeekNameId();
auto entity_name_id = SemIR::EntityNameId::None;

const DeclIntroducerState& introducer =
context.decl_introducer_state_stack().innermost();
if (introducer.kind == Lex::TokenKind::Let) {
Copy link
Contributor

@zygoloid zygoloid Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On rereading this I'm not sure I understand what's going on here: what other kinds of compile-time bindings are there besides Let? (I was first wondering if we could drop the "`let` " from the TODO diagnostic below, but maybe I'm missing something here.)

// 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 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::FunctionDecl>()) {
context.TODO(
node_id,
"`let` compile time binding outside function or interface");
entity_name_id = context.entity_names().Add(
{.name_id = name_id,
.parent_scope_id = context.scope_stack().PeekNameScopeId()});
}
}
}

// This compile time binding is added but not pushed because we don't have the
// BindSymbolicName for it until we get to the CompileTimeBindingPatternId
// node. This leaves the CompileTimeBinding stack in a fragile state while the
// binding's facet type is checked, but only the index is used during type
// checking. The instruction wouldn't be needed until building a generic,
// which we don't do in this scope.
//
// We avoid making a CompileTimeBinding in the case where a compile-time
// binding is disallowed. In that case, we already constructed an
// `entity_name_id` above.
Comment on lines +292 to +295
Copy link
Contributor

@zygoloid zygoloid Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//
// We avoid making a CompileTimeBinding in the case where a compile-time
// binding is disallowed. In that case, we already constructed an
// `entity_name_id` above.

(Given that we don't even get here any more in the case this comment is talking about.)

if (!entity_name_id.has_value()) {
auto bind_index = context.scope_stack().AddCompileTimeBinding();
entity_name_id = context.entity_names().AddSymbolicBindingName(
name_id, context.scope_stack().PeekNameScopeId(), bind_index,
is_template);
}

// TODO: Construct a SymbolicBindingType for `entity_name_id` instead of a
// BindSymbolicName for a `PeriodSelf` EntityName.
//
// The `.Self` must have a type of `FacetType`, so that it gets wrapped in
// `FacetAccessType` when used in a type position, such as in `U:! I(.Self)`.
// This allows substitution with other facet values without requiring an
Expand All @@ -244,7 +310,13 @@ auto HandleParseNode(Context& context,
{.type_id = SemIR::TypeType::TypeId, .facet_type_id = facet_type_id});
auto type_id = context.types().GetTypeIdForTypeConstantId(const_id);

// Make a scope to contain the `.Self` facet value for use in the type of the
// compile time binding. This is popped when handling the
// CompileTimeBindingPatternId.
context.scope_stack().PushForSameRegion();
MakePeriodSelfFacetValue(context, type_id);

context.node_stack().Push(node_id, entity_name_id);
return true;
}

Expand All @@ -254,30 +326,29 @@ auto HandleParseNode(Context& context,
// CompileTimeBindingPatternStart.
context.scope_stack().Pop();

auto [type_node, type_inst_id] = context.node_stack().PopExprWithNodeId();

auto entity_name_id =
context.node_stack()
.Pop<Parse::NodeKind::CompileTimeBindingPatternStart>();

// The NameId was already used to construct the `entity_name_id`.
auto [name_node, _] = context.node_stack().PopNameWithNodeId();

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 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::FunctionDecl>()) {
context.TODO(
node_id,
"`let` compile time binding outside function or interface");
node_kind = Parse::NodeKind::LetBindingPattern;
}
}
if (!context.entity_names().Get(entity_name_id).bind_index().has_value()) {
// This indicates that CompileTimeBindingPatternStartId found a `let` in an
// incorrect scope. We treat them as runtime bindings instead of compile
// time bindings, since we avoided introducing a CompileTimeBindingIndex
// that won't be used.
node_kind = Parse::NodeKind::LetBindingPattern;
}

return HandleAnyBindingPattern(context, node_id, node_kind);
auto success =
HandleAnyBindingPattern(context, node_id, name_node, entity_name_id,
type_node, type_inst_id, node_kind);

return success;
}

auto HandleParseNode(Context& context,
Expand Down
10 changes: 9 additions & 1 deletion toolchain/check/node_stack.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,9 @@ class NodeStack {
return PopWithNodeId<SemIR::NameId>();
}

// Peeks a name from the top of the stack.
auto PeekNameId() const -> SemIR::NameId;

// Pops the top of the stack and returns the node_id and the ID.
template <const Parse::NodeKind& RequiredParseKind>
auto PopWithNodeId() -> auto {
Expand Down Expand Up @@ -442,6 +445,8 @@ class NodeStack {
case Parse::NodeKind::DefaultLibrary:
case Parse::NodeKind::LibraryName:
return Id::KindFor<SemIR::LibraryNameId>();
case Parse::NodeKind::CompileTimeBindingPatternStart:
return Id::KindFor<SemIR::EntityNameId>();
case Parse::NodeKind::BuiltinName:
case Parse::NodeKind::ChoiceIntroducer:
case Parse::NodeKind::ClassIntroducer:
Expand Down Expand Up @@ -481,7 +486,6 @@ class NodeStack {
case Parse::NodeKind::CallExprComma:
case Parse::NodeKind::ChoiceAlternativeListComma:
case Parse::NodeKind::CodeBlock:
case Parse::NodeKind::CompileTimeBindingPatternStart:
case Parse::NodeKind::ContinueStatementStart:
case Parse::NodeKind::CorePackageName:
case Parse::NodeKind::ExportIntroducer:
Expand Down Expand Up @@ -640,6 +644,10 @@ inline auto NodeStack::PopExprWithNodeId()
return PopWithNodeId<Parse::NodeCategory::Expr>();
}

inline auto NodeStack::PeekNameId() const -> SemIR::NameId {
return Peek<Id::KindFor<SemIR::NameId>()>();
}

inline auto NodeStack::PeekPattern() const -> SemIR::InstId {
return Peek<Id::KindFor<SemIR::InstId>()>();
}
Expand Down
26 changes: 17 additions & 9 deletions toolchain/check/pattern.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,22 @@ auto EndSubpatternAsNonExpr(Context& context) -> void {

auto AddBindingPattern(Context& context, SemIR::LocId name_loc,
SemIR::NameId name_id, SemIR::TypeId type_id,
SemIR::ExprRegionId type_region_id, bool is_generic,
bool is_template) -> BindingPatternInfo {
auto entity_name_id = context.entity_names().AddSymbolicBindingName(
name_id, context.scope_stack().PeekNameScopeId(),
is_generic ? context.scope_stack().AddCompileTimeBinding()
: SemIR::CompileTimeBindIndex::None,
is_template);
SemIR::ExprRegionId type_region_id)
-> BindingPatternInfo {
auto entity_name_id = context.entity_names().Add(
{.name_id = name_id,
.parent_scope_id = context.scope_stack().PeekNameScopeId()});
return AddBindingPatternWithEntityName(context, name_loc, entity_name_id,
type_id, type_region_id);
}

auto AddBindingPatternWithEntityName(Context& context, SemIR::LocId name_loc,
SemIR::EntityNameId entity_name_id,
SemIR::TypeId type_id,
SemIR::ExprRegionId type_region_id)
-> BindingPatternInfo {
auto& entity_name = context.entity_names().Get(entity_name_id);
bool is_generic = entity_name.bind_index().has_value();

auto bind_id = SemIR::InstId::None;
if (is_generic) {
Expand Down Expand Up @@ -138,8 +147,7 @@ auto AddSelfParamPattern(Context& context, SemIR::LocId loc_id,
SemIR::TypeId type_id) -> SemIR::InstId {
SemIR::InstId pattern_id =
AddBindingPattern(context, loc_id, SemIR::NameId::SelfValue, type_id,
type_expr_region_id, /*is_generic=*/false,
/*is_template=*/false)
type_expr_region_id)
.pattern_id;

pattern_id = AddPatternInst<SemIR::ValueParamPattern>(
Expand Down
19 changes: 15 additions & 4 deletions toolchain/check/pattern.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,23 @@ struct BindingPatternInfo {

// TODO: Add EndSubpatternAsPattern, when needed.

// Creates a binding pattern. Returns the binding pattern and the bind name
// instruction.
// Creates a binding pattern and EntityName for a `name_id`. Returns the binding
// pattern and the bind name instruction.
//
// To make a generic binding with a CompileTimeBindIndex, construct the
// EntityName and use `AddBindingPatternWithEntityName()`.
auto AddBindingPattern(Context& context, SemIR::LocId name_loc,
SemIR::NameId name_id, SemIR::TypeId type_id,
SemIR::ExprRegionId type_region_id, bool is_generic,
bool is_template) -> BindingPatternInfo;
SemIR::ExprRegionId type_region_id)
-> BindingPatternInfo;

// Creates a binding pattern with a pre-created EntityName. Returns the binding
// pattern and the bind name instruction.
auto AddBindingPatternWithEntityName(Context& context, SemIR::LocId name_loc,
SemIR::EntityNameId entity_name_id,
SemIR::TypeId type_id,
SemIR::ExprRegionId type_region_id)
-> BindingPatternInfo;

// Creates storage for `var` patterns nested within the given pattern at the
// current location in the output SemIR. For a `returned var`, this
Expand Down
Loading
Loading