carbon-language/carbon-lang
1
0
Fork 0
mirror of https://github.com/carbon-language/carbon-lang.git synced 2026-10-06 08:14:43 +01:00
Code Issues Packages Projects Releases 10 Wiki Activity

Remove ArgKinds to encourage safer coding patterns (#5212)

Browse Source 
#5171 ran into an issue where the wrong kind was associated with an arg
(`auto arg1 = RefineOperand(context, loc_id, arg0_kind,
action.arg1());`). This PR is trying to reduce risk of similar errors by
replaced `ArgKinds()` with instead an `ArgAndKind` structure and
corresponding accessors.

A couple things I considered and discarded were:

- Adding `CARBON_KIND_SWITCH` support (in this PR -- see #5216).
- The particular way that `ForCase` works would need to change, and I
was hesitant to do that here.
- But this is why I did add `As` to `ArgAndKind`, because it had me
thinking in that direction.
- Trying to make wrapper functions like `MutateArgs(callback_fn);`. This
kind of approach gets a little messy due to some of the conditional
passes, and in particular the reverse-iteration done for `PopOperand` in
subst.cpp
- Making something like `args_and_kinds() -> std::array<ArgAndKind, 2>`.
There's one spot where iteration is already set up as a loop, but for
others it felt a little convoluted with less gain than
`MutateArgs`-style things.

I'm not sure if there's a better way to set up the table generators, I
might keep tinkering with those for ideas.
This commit is contained in:
Jon Ross-Perkins
2025-03-28 23:24:36 +00:00
committed by GitHub
parent 2973cafd60
commit 4cb61ae4e1
8 changed files with 204 additions and 161 deletions
Download Patch File Download Diff File Expand all files Collapse all files
toolchain/check/subst.cpp
+46 -40
View File
@@ -60,7 +60,7 @@ class Worklist {
// Pushes the specified operand onto the worklist.
static auto PushOperand(Context& context, Worklist& worklist,
SemIR::IdKind kind, int32_t arg) -> void {
SemIR::Inst::ArgAndKind arg) -> void {
auto push_block = [&](SemIR::InstBlockId block_id) {
for (auto inst_id :
context.inst_blocks().Get(SemIR::InstBlockId(block_id))) {
@@ -74,45 +74,50 @@ static auto PushOperand(Context& context, Worklist& worklist,
}
};
switch (kind) {
switch (arg.kind) {
case SemIR::IdKind::For<SemIR::InstId>:
if (auto inst_id = arg.As<SemIR::InstId>(); inst_id.has_value()) {
worklist.Push(inst_id);
}
break;
case SemIR::IdKind::For<SemIR::MetaInstId>:
if (SemIR::InstId inst_id(arg); inst_id.has_value()) {
if (auto inst_id = arg.As<SemIR::MetaInstId>(); inst_id.has_value()) {
worklist.Push(inst_id);
}
break;
case SemIR::IdKind::For<SemIR::TypeId>:
if (SemIR::TypeId type_id(arg); type_id.has_value()) {
if (auto type_id = arg.As<SemIR::TypeId>(); type_id.has_value()) {
worklist.Push(context.types().GetInstId(type_id));
}
break;
case SemIR::IdKind::For<SemIR::InstBlockId>:
push_block(SemIR::InstBlockId(arg));
push_block(arg.As<SemIR::InstBlockId>());
break;
case SemIR::IdKind::For<SemIR::StructTypeFieldsId>: {
for (auto field :
context.struct_type_fields().Get(SemIR::StructTypeFieldsId(arg))) {
for (auto field : context.struct_type_fields().Get(
arg.As<SemIR::StructTypeFieldsId>())) {
worklist.Push(context.types().GetInstId(field.type_id));
}
break;
}
case SemIR::IdKind::For<SemIR::TypeBlockId>:
for (auto type_id : context.type_blocks().Get(SemIR::TypeBlockId(arg))) {
for (auto type_id :
context.type_blocks().Get(arg.As<SemIR::TypeBlockId>())) {
worklist.Push(context.types().GetInstId(type_id));
}
break;
case SemIR::IdKind::For<SemIR::SpecificId>:
push_specific(SemIR::SpecificId(arg));
push_specific(arg.As<SemIR::SpecificId>());
break;
case SemIR::IdKind::For<SemIR::SpecificInterfaceId>: {
auto interface =
context.specific_interfaces().Get(SemIR::SpecificInterfaceId(arg));
auto interface = context.specific_interfaces().Get(
arg.As<SemIR::SpecificInterfaceId>());
push_specific(interface.specific_id);
break;
}
case SemIR::IdKind::For<SemIR::FacetTypeId>: {
const auto& facet_type_info =
context.facet_types().Get(SemIR::FacetTypeId(arg));
context.facet_types().Get(arg.As<SemIR::FacetTypeId>());
for (auto interface : facet_type_info.impls_constraints) {
push_specific(interface.specific_id);
}
@@ -137,16 +142,14 @@ static auto PushOperand(Context& context, Worklist& worklist,
static auto ExpandOperands(Context& context, Worklist& worklist,
SemIR::InstId inst_id) -> void {
auto inst = context.insts().Get(inst_id);
auto kinds = inst.ArgKinds();
PushOperand(context, worklist, SemIR::IdKind::For<SemIR::TypeId>,
inst.type_id().index);
PushOperand(context, worklist, kinds.first, inst.arg0());
PushOperand(context, worklist, kinds.second, inst.arg1());
PushOperand(context, worklist, inst.type_id_and_kind());
PushOperand(context, worklist, inst.arg0_and_kind());
PushOperand(context, worklist, inst.arg1_and_kind());
}
// Pops the specified operand from the worklist and returns it.
static auto PopOperand(Context& context, Worklist& worklist, SemIR::IdKind kind,
int32_t arg) -> int32_t {
static auto PopOperand(Context& context, Worklist& worklist,
SemIR::Inst::ArgAndKind arg) -> int32_t {
auto pop_block_id = [&](SemIR::InstBlockId old_inst_block_id) {
auto size = context.inst_blocks().Get(old_inst_block_id).size();
SemIR::CopyOnWriteInstBlock new_inst_block(context.sem_ir(),
@@ -166,27 +169,33 @@ static auto PopOperand(Context& context, Worklist& worklist, SemIR::IdKind kind,
return context.specifics().GetOrAdd(specific.generic_id, args_id);
};
switch (kind) {
case SemIR::IdKind::For<SemIR::InstId>:
case SemIR::IdKind::For<SemIR::MetaInstId>: {
SemIR::InstId inst_id(arg);
switch (arg.kind) {
case SemIR::IdKind::For<SemIR::InstId>: {
auto inst_id = arg.As<SemIR::InstId>();
if (!inst_id.has_value()) {
return arg;
return arg.value;
}
return worklist.Pop().index;
}
case SemIR::IdKind::For<SemIR::MetaInstId>: {
auto inst_id = arg.As<SemIR::MetaInstId>();
if (!inst_id.has_value()) {
return arg.value;
}
return worklist.Pop().index;
}
case SemIR::IdKind::For<SemIR::TypeId>: {
SemIR::TypeId type_id(arg);
auto type_id = arg.As<SemIR::TypeId>();
if (!type_id.has_value()) {
return arg;
return arg.value;
}
return context.types().GetTypeIdForTypeInstId(worklist.Pop()).index;
}
case SemIR::IdKind::For<SemIR::InstBlockId>: {
return pop_block_id(SemIR::InstBlockId(arg)).index;
return pop_block_id(arg.As<SemIR::InstBlockId>()).index;
}
case SemIR::IdKind::For<SemIR::StructTypeFieldsId>: {
SemIR::StructTypeFieldsId old_fields_id(arg);
auto old_fields_id = arg.As<SemIR::StructTypeFieldsId>();
auto old_fields = context.struct_type_fields().Get(old_fields_id);
SemIR::CopyOnWriteStructTypeFieldsBlock new_fields(context.sem_ir(),
old_fields_id);
@@ -198,7 +207,7 @@ static auto PopOperand(Context& context, Worklist& worklist, SemIR::IdKind kind,
return new_fields.GetCanonical().index;
}
case SemIR::IdKind::For<SemIR::TypeBlockId>: {
SemIR::TypeBlockId old_type_block_id(arg);
auto old_type_block_id = arg.As<SemIR::TypeBlockId>();
auto size = context.type_blocks().Get(old_type_block_id).size();
SemIR::CopyOnWriteTypeBlock new_type_block(context.sem_ir(),
old_type_block_id);
@@ -209,11 +218,11 @@ static auto PopOperand(Context& context, Worklist& worklist, SemIR::IdKind kind,
return new_type_block.GetCanonical().index;
}
case SemIR::IdKind::For<SemIR::SpecificId>: {
return pop_specific(SemIR::SpecificId(arg)).index;
return pop_specific(arg.As<SemIR::SpecificId>()).index;
}
case SemIR::IdKind::For<SemIR::SpecificInterfaceId>: {
auto interface =
context.specific_interfaces().Get(SemIR::SpecificInterfaceId(arg));
auto interface = context.specific_interfaces().Get(
arg.As<SemIR::SpecificInterfaceId>());
auto specific_id = pop_specific(interface.specific_id);
return context.specific_interfaces()
.Add({
@@ -224,7 +233,7 @@ static auto PopOperand(Context& context, Worklist& worklist, SemIR::IdKind kind,
}
case SemIR::IdKind::For<SemIR::FacetTypeId>: {
const auto& old_facet_type_info =
context.facet_types().Get(SemIR::FacetTypeId(arg));
context.facet_types().Get(arg.As<SemIR::FacetTypeId>());
SemIR::FacetTypeInfo new_facet_type_info;
// Since these were added to a stack, we get them back in reverse order.
new_facet_type_info.rewrite_constraints.resize(
@@ -252,7 +261,7 @@ static auto PopOperand(Context& context, Worklist& worklist, SemIR::IdKind kind,
return context.facet_types().Add(new_facet_type_info).index;
}
default:
return arg;
return arg.value;
}
}
@@ -261,14 +270,11 @@ static auto PopOperand(Context& context, Worklist& worklist, SemIR::IdKind kind,
static auto Rebuild(Context& context, Worklist& worklist, SemIR::InstId inst_id,
const SubstInstCallbacks& callbacks) -> SemIR::InstId {
auto inst = context.insts().Get(inst_id);
auto kinds = inst.ArgKinds();
// Note that we pop in reverse order because we pushed them in forwards order.
int32_t arg1 = PopOperand(context, worklist, kinds.second, inst.arg1());
int32_t arg0 = PopOperand(context, worklist, kinds.first, inst.arg0());
int32_t type_id =
PopOperand(context, worklist, SemIR::IdKind::For<SemIR::TypeId>,
inst.type_id().index);
int32_t arg1 = PopOperand(context, worklist, inst.arg1_and_kind());
int32_t arg0 = PopOperand(context, worklist, inst.arg0_and_kind());
int32_t type_id = PopOperand(context, worklist, inst.type_id_and_kind());
if (type_id == inst.type_id().index && arg0 == inst.arg0() &&
arg1 == inst.arg1()) {
return callbacks.ReuseUnchanged(inst_id);