Skip to content

Support for lowering references to imported vars. #5513

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.

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

Merged
merged 15 commits into from
May 29, 2025
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
18 changes: 18 additions & 0 deletions toolchain/check/eval_inst.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "toolchain/check/type.h"
#include "toolchain/check/type_completion.h"
#include "toolchain/diagnostics/diagnostic.h"
#include "toolchain/sem_ir/expr_info.h"
#include "toolchain/sem_ir/ids.h"
#include "toolchain/sem_ir/pattern.h"
#include "toolchain/sem_ir/typed_insts.h"
Expand Down Expand Up @@ -102,6 +103,23 @@ auto EvalConstantInst(Context& context, SemIR::BindAlias inst)
context.constant_values().Get(inst.value_id));
}

auto EvalConstantInst(Context& context, SemIR::BindName inst)
-> ConstantEvalResult {
if (!inst.value_id.has_value()) {
return ConstantEvalResult::NotConstant;
}

// A reference binding evaluates to the value it's bound to.
if (SemIR::IsReferenceCategory(
SemIR::GetExprCategory(context.sem_ir(), inst.value_id))) {
return ConstantEvalResult::Existing(
context.constant_values().Get(inst.value_id));
}

// Non-`:!` value bindings are not constant.
return ConstantEvalResult::NotConstant;
}

auto EvalConstantInst(Context& /*context*/, SemIR::BindValue /*inst*/)
-> ConstantEvalResult {
// TODO: Handle this once we've decided how to represent constant values of
Expand Down
2 changes: 2 additions & 0 deletions toolchain/check/eval_inst.h
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ constexpr auto ConstantKindHasEvalConstantInst(SemIR::InstConstantKind kind)
case SemIR::InstConstantKind::SymbolicOnly:
case SemIR::InstConstantKind::SymbolicOrReference:
case SemIR::InstConstantKind::Conditional:
case SemIR::InstConstantKind::ConditionalUnique:
return true;
}
}
Expand Down Expand Up @@ -163,6 +164,7 @@ auto EvalConstantInst() -> void = delete;
// - InstConstantKind::SymbolicOnly
// - InstConstantKind::SymbolicOrReference
// - InstConstantKind::Conditional
// - InstConstantKind::ConditionalUnique
//
// ... except for cases where the result of evaluation depends on the evaluation
// context itself. Those cases are handled by explicit specialization of
Expand Down
155 changes: 127 additions & 28 deletions toolchain/check/import_ref.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "toolchain/check/inst.h"
#include "toolchain/check/name_lookup.h"
#include "toolchain/check/type.h"
#include "toolchain/check/type_completion.h"
#include "toolchain/parse/node_ids.h"
#include "toolchain/sem_ir/constant.h"
#include "toolchain/sem_ir/file.h"
Expand Down Expand Up @@ -1238,8 +1239,11 @@ template <typename InstT>
static auto ResolveAsUnique(ImportRefResolver& resolver,
SemIR::InstId import_inst_id, InstT inst)
-> ResolveResult {
static_assert(InstT::Kind.constant_kind() == SemIR::InstConstantKind::Unique,
"Use ResolveAsDeduplicated");
static_assert(
InstT::Kind.constant_kind() == SemIR::InstConstantKind::Unique ||
InstT::Kind.constant_kind() ==
SemIR::InstConstantKind::ConditionalUnique,
"Use ResolveAsDeduplicated");
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be a requires instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it could be, but we can't provide a custom error message so easily that way. Are you thinking that we give the two ResolveAs... functions the same name again? If we keep them separate, a diagnostic that mentions the other function name seems at least a little valuable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm just ambivalent about the ResolveAsDeduplicated, on two ends:

  • I think people will figure it out from a simple compile error, a comment seems equivalent (this isn't a high-use API).
  • I don't see why the name changed, when requires could have achieved equivalent enforcement. But, I didn't suggest/review it so I don't want to push too much.

I mainly noted it, though, since I've recently bumped into issues with static_assert and having to migrate things to requires. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

But note, I also am hesitant to just churn code when I don't see a benefit. It's not like I think overloading would really fix an issue. I just am losing my love for static_assert when requires would suffice. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to a concept.

CARBON_CHECK(!resolver.HasNewWork());
auto inst_id = AddPlaceholderImportedInst(resolver, import_inst_id, inst);
auto const_id = SetConstantValue(resolver.local_context(), inst_id, inst);
Expand Down Expand Up @@ -1455,24 +1459,6 @@ static auto TryResolveTypedInst(ImportRefResolver& resolver,
.index = inst.index});
}

static auto TryResolveTypedInst(ImportRefResolver& resolver, SemIR::Vtable inst)
-> ResolveResult {
auto type_const_id = GetLocalConstantId(resolver, inst.type_id);
auto virtual_functions =
GetLocalInstBlockContents(resolver, inst.virtual_functions_id);
if (resolver.HasNewWork()) {
return ResolveResult::Retry();
}

auto virtual_functions_id = GetLocalCanonicalInstBlockId(
resolver, inst.virtual_functions_id, virtual_functions);
return ResolveAsDeduplicated<SemIR::Vtable>(
resolver,
{.type_id = resolver.local_context().types().GetTypeIdForTypeConstantId(
type_const_id),
.virtual_functions_id = virtual_functions_id});
}

static auto TryResolveTypedInst(ImportRefResolver& resolver,
SemIR::BindAlias inst) -> ResolveResult {
auto value_id = GetLocalConstantId(resolver, inst.value_id);
Expand Down Expand Up @@ -1507,16 +1493,18 @@ static auto TryResolveTypedInst(ImportRefResolver& resolver,
BindingPatternT inst,
SemIR::InstId import_inst_id) -> ResolveResult {
auto type_const_id = GetLocalConstantId(resolver, inst.type_id);
const auto& import_entity_name =
resolver.import_entity_names().Get(inst.entity_name_id);
auto parent_scope_id =
GetLocalNameScopeId(resolver, import_entity_name.parent_scope_id);
if (resolver.HasNewWork()) {
return ResolveResult::Retry();
}

const auto& import_entity_name =
resolver.import_entity_names().Get(inst.entity_name_id);
auto name_id = GetLocalNameId(resolver, import_entity_name.name_id);
auto entity_name_id = resolver.local_entity_names().Add(
{.name_id = name_id,
.parent_scope_id = SemIR::NameScopeId::None,
.parent_scope_id = parent_scope_id,
.bind_index_value = import_entity_name.bind_index().index,
.is_template = import_entity_name.is_template});
return ResolveAsUnique<BindingPatternT>(
Expand Down Expand Up @@ -2622,6 +2610,27 @@ static auto TryResolveTypedInst(ImportRefResolver& resolver,
{.type_id = type_id, .callee_id = callee_id, .specific_id = specific_id});
}

static auto TryResolveTypedInst(ImportRefResolver& resolver,
SemIR::StructAccess inst) -> ResolveResult {
auto type_id = GetLocalConstantId(resolver, inst.type_id);
auto struct_id = GetLocalConstantInstId(resolver, inst.struct_id);
if (resolver.HasNewWork()) {
return ResolveResult::Retry();
}

// A `struct_access` constant requires its struct operand to have a complete
// type.
CompleteTypeOrCheckFail(resolver.local_context(),
resolver.local_insts().Get(struct_id).type_id());

return ResolveAsDeduplicated<SemIR::StructAccess>(
resolver,
{.type_id =
resolver.local_context().types().GetTypeIdForTypeConstantId(type_id),
.struct_id = struct_id,
.index = inst.index});
}

static auto TryResolveTypedInst(ImportRefResolver& resolver,
SemIR::StructType inst) -> ResolveResult {
CARBON_CHECK(inst.type_id == SemIR::TypeType::TypeId);
Expand Down Expand Up @@ -2668,6 +2677,44 @@ static auto TryResolveTypedInst(ImportRefResolver& resolver,
GetLocalCanonicalInstBlockId(resolver, inst.elements_id, elems)});
}

static auto TryResolveTypedInst(ImportRefResolver& resolver,
SemIR::TupleAccess inst) -> ResolveResult {
auto type_id = GetLocalConstantId(resolver, inst.type_id);
auto tuple_id = GetLocalConstantInstId(resolver, inst.tuple_id);
if (resolver.HasNewWork()) {
return ResolveResult::Retry();
}

// A `tuple_access` constant requires its struct operand to have a complete
// type.
CompleteTypeOrCheckFail(resolver.local_context(),
resolver.local_insts().Get(tuple_id).type_id());

return ResolveAsDeduplicated<SemIR::TupleAccess>(
resolver,
{.type_id =
resolver.local_context().types().GetTypeIdForTypeConstantId(type_id),
.tuple_id = tuple_id,
.index = inst.index});
}

static auto TryResolveTypedInst(ImportRefResolver& resolver,
SemIR::TuplePattern inst,
SemIR::InstId import_inst_id) -> ResolveResult {
auto type_const_id = GetLocalConstantId(resolver, inst.type_id);
auto elements = GetLocalInstBlockContents(resolver, inst.elements_id);
if (resolver.HasNewWork()) {
return ResolveResult::Retry();
}

return ResolveAsUnique<SemIR::TuplePattern>(
resolver, import_inst_id,
{.type_id = resolver.local_context().types().GetTypeIdForTypeConstantId(
type_const_id),
.elements_id =
GetLocalCanonicalInstBlockId(resolver, inst.elements_id, elements)});
}

static auto TryResolveTypedInst(ImportRefResolver& resolver,
SemIR::TupleType inst) -> ResolveResult {
CARBON_CHECK(inst.type_id == SemIR::TypeType::TypeId);
Expand Down Expand Up @@ -2742,6 +2789,40 @@ static auto TryResolveTypedInst(ImportRefResolver& resolver,
.subpattern_id = subpattern_id});
}

static auto TryResolveTypedInst(ImportRefResolver& resolver,
SemIR::VarStorage inst,
SemIR::InstId import_inst_id) -> ResolveResult {
auto type_const_id = GetLocalConstantId(resolver, inst.type_id);
auto pattern_id = GetLocalConstantInstId(resolver, inst.pattern_id);
if (resolver.HasNewWork()) {
return ResolveResult::Retry();
}

return ResolveAsUnique<SemIR::VarStorage>(
resolver, import_inst_id,
{.type_id = resolver.local_context().types().GetTypeIdForTypeConstantId(
type_const_id),
.pattern_id = pattern_id});
}

static auto TryResolveTypedInst(ImportRefResolver& resolver, SemIR::Vtable inst)
-> ResolveResult {
auto type_const_id = GetLocalConstantId(resolver, inst.type_id);
auto virtual_functions =
GetLocalInstBlockContents(resolver, inst.virtual_functions_id);
if (resolver.HasNewWork()) {
return ResolveResult::Retry();
}

auto virtual_functions_id = GetLocalCanonicalInstBlockId(
resolver, inst.virtual_functions_id, virtual_functions);
return ResolveAsDeduplicated<SemIR::Vtable>(
resolver,
{.type_id = resolver.local_context().types().GetTypeIdForTypeConstantId(
type_const_id),
.virtual_functions_id = virtual_functions_id});
}

// Tries to resolve the InstId, returning a canonical constant when ready, or
// `None` if more has been added to the stack. This is the same as
// TryResolveInst, except that it may resolve symbolic constants as canonical
Expand Down Expand Up @@ -2789,10 +2870,6 @@ static auto TryResolveInstCanonical(ImportRefResolver& resolver,
case CARBON_KIND(SemIR::BindingPattern inst): {
return TryResolveTypedInst(resolver, inst, inst_id);
}
case SemIR::BindName::Kind: {
// TODO: Should we be resolving BindNames at all?
return ResolveResult::Done(SemIR::ConstantId::NotConstant);
}
case CARBON_KIND(SemIR::BindSymbolicName inst): {
return TryResolveTypedInst(resolver, inst);
}
Expand Down Expand Up @@ -2898,6 +2975,9 @@ static auto TryResolveInstCanonical(ImportRefResolver& resolver,
case CARBON_KIND(SemIR::SpecificImplFunction inst): {
return TryResolveTypedInst(resolver, inst);
}
case CARBON_KIND(SemIR::StructAccess inst): {
return TryResolveTypedInst(resolver, inst);
}
case CARBON_KIND(SemIR::StructType inst): {
return TryResolveTypedInst(resolver, inst);
}
Expand All @@ -2907,6 +2987,12 @@ static auto TryResolveInstCanonical(ImportRefResolver& resolver,
case CARBON_KIND(SemIR::SymbolicBindingPattern inst): {
return TryResolveTypedInst(resolver, inst, inst_id);
}
case CARBON_KIND(SemIR::TupleAccess inst): {
return TryResolveTypedInst(resolver, inst);
}
case CARBON_KIND(SemIR::TuplePattern inst): {
return TryResolveTypedInst(resolver, inst, inst_id);
}
case CARBON_KIND(SemIR::TupleType inst): {
return TryResolveTypedInst(resolver, inst);
}
Expand All @@ -2922,13 +3008,26 @@ static auto TryResolveInstCanonical(ImportRefResolver& resolver,
case CARBON_KIND(SemIR::VarPattern inst): {
return TryResolveTypedInst(resolver, inst, inst_id);
}
case CARBON_KIND(SemIR::VarStorage inst): {
return TryResolveTypedInst(resolver, inst, inst_id);
}
case CARBON_KIND(SemIR::Vtable inst): {
return TryResolveTypedInst(resolver, inst);
}
default: {
auto inst_constant_id = resolver.import_constant_values().Get(inst_id);
if (!inst_constant_id.is_constant()) {
// TODO: Import of non-constant BindNames happens when importing `let`
// declarations.
CARBON_CHECK(untyped_inst.Is<SemIR::BindName>(),
"TryResolveInst on non-constant instruction {0}",
untyped_inst);
return ResolveResult::Done(SemIR::ConstantId::NotConstant);
}

// This instruction might have a constant value of a different kind.
auto constant_inst_id =
resolver.import_constant_values().GetConstantInstId(inst_id);
resolver.import_constant_values().GetInstId(inst_constant_id);
if (constant_inst_id == inst_id) {
// Produce a diagnostic to provide a source location with the CHECK
// failure.
Expand Down
2 changes: 1 addition & 1 deletion toolchain/check/testdata/alias/builtins.carbon
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ let a_test: bool = a;
// CHECK:STDOUT: %.loc12_13.2: type = value_of_initializer %bool.make_type [concrete = bool]
// CHECK:STDOUT: %.loc12_13.3: type = converted %bool.make_type, %.loc12_13.2 [concrete = bool]
// CHECK:STDOUT: }
// CHECK:STDOUT: %a_test: bool = bind_name a_test, <error>
// CHECK:STDOUT: %a_test: bool = bind_name a_test, <error> [concrete = <error>]
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: fn @__global_init() {
Expand Down
13 changes: 7 additions & 6 deletions toolchain/check/testdata/alias/no_prelude/export_name.carbon
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ var d: D* = &c;
// CHECK:STDOUT: }
// CHECK:STDOUT: %d.var: ref %C = var %d.var_patt [concrete]
// CHECK:STDOUT: %D.ref: type = name_ref D, imports.%Main.D [concrete = constants.%C]
// CHECK:STDOUT: %d: ref %C = bind_name d, %d.var
// CHECK:STDOUT: %d: ref %C = bind_name d, %d.var [concrete = %d.var]
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: class @C [from "export.carbon"] {
Expand Down Expand Up @@ -230,7 +230,7 @@ var d: D* = &c;
// CHECK:STDOUT: }
// CHECK:STDOUT: %c.var: ref <error> = var %c.var_patt [concrete = <error>]
// CHECK:STDOUT: %C.ref: <error> = name_ref C, <error> [concrete = <error>]
// CHECK:STDOUT: %c: <error> = bind_name c, <error>
// CHECK:STDOUT: %c: <error> = bind_name c, <error> [concrete = <error>]
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: fn @__global_init() {
Expand All @@ -250,6 +250,7 @@ var d: D* = &c;
// CHECK:STDOUT: %C.val: %C = struct_value () [concrete]
// CHECK:STDOUT: %ptr.019: type = ptr_type %C [concrete]
// CHECK:STDOUT: %pattern_type.44a: type = pattern_type %ptr.019 [concrete]
// CHECK:STDOUT: %addr: %ptr.019 = addr_of file.%c.var [concrete]
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: imports {
Expand All @@ -273,7 +274,7 @@ var d: D* = &c;
// CHECK:STDOUT: }
// CHECK:STDOUT: %c.var: ref %C = var %c.var_patt [concrete]
// CHECK:STDOUT: %C.ref: type = name_ref C, imports.%Main.C [concrete = constants.%C]
// CHECK:STDOUT: %c: ref %C = bind_name c, %c.var
// CHECK:STDOUT: %c: ref %C = bind_name c, %c.var [concrete = %c.var]
// CHECK:STDOUT: name_binding_decl {
// CHECK:STDOUT: %d.patt: %pattern_type.44a = binding_pattern d [concrete]
// CHECK:STDOUT: %d.var_patt: %pattern_type.44a = var_pattern %d.patt [concrete]
Expand All @@ -283,7 +284,7 @@ var d: D* = &c;
// CHECK:STDOUT: %D.ref: type = name_ref D, imports.%Main.D [concrete = constants.%C]
// CHECK:STDOUT: %ptr: type = ptr_type %D.ref [concrete = constants.%ptr.019]
// CHECK:STDOUT: }
// CHECK:STDOUT: %d: ref %ptr.019 = bind_name d, %d.var
// CHECK:STDOUT: %d: ref %ptr.019 = bind_name d, %d.var [concrete = %d.var]
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: class @C [from "export_orig.carbon"] {
Expand All @@ -299,8 +300,8 @@ var d: D* = &c;
// CHECK:STDOUT: %.loc7_13.2: init %C = class_init (), file.%c.var [concrete = constants.%C.val]
// CHECK:STDOUT: %.loc7_1: init %C = converted %.loc7_13.1, %.loc7_13.2 [concrete = constants.%C.val]
// CHECK:STDOUT: assign file.%c.var, %.loc7_1
// CHECK:STDOUT: %c.ref: ref %C = name_ref c, file.%c
// CHECK:STDOUT: %addr: %ptr.019 = addr_of %c.ref
// CHECK:STDOUT: %c.ref: ref %C = name_ref c, file.%c [concrete = file.%c.var]
// CHECK:STDOUT: %addr: %ptr.019 = addr_of %c.ref [concrete = constants.%addr]
// CHECK:STDOUT: assign file.%d.var, %addr
// CHECK:STDOUT: return
// CHECK:STDOUT: }
Expand Down
Loading
Loading