Refactor Impl construction to clarify and reduce extraneous work #6420

danakj · 2025-11-21T18:32:41Z

We were requiring the facet type to be complete in two different places, but this was not necessary. Now we only require it to be complete in a single place when checking the definition. If it was required earlier for a rewrite constraint, it would have been already checked and diagnosed in member lookup for the rewrite.

In the process reorganize and rename code to help clarify the relationships and behaviour between handle_impl.cpp and impl.cpp.

Move the diagnosing of extend impl T as to the handler of the T, similar to where it's diagnosed for require decls. This helps simplify the code, avoiding a bunch of looking through parse nodes for specific types. Make them both handle errors in the self type in a similar manner, avoiding a second diagnostic.

danakj · 2025-11-21T18:33:25Z

This is based on #6385, the first commit is less-completing-impl-as.

danakj · 2025-11-24T15:22:44Z

This is based on #6385, the first commit is less-completing-impl-as.

Rebased to trunk.

josh11b

Sorry for the delay in reviewing.

This PR seems a bit large, it ended up being pretty difficult to sort through all the changes. It would have helped a lot to break this into smaller changes.

josh11b · 2025-11-25T20:03:37Z

toolchain/check/name_scope.cpp

+  if (!class_decl) {
+    return std::nullopt;
+  }
+  // TODO: This is also valid in a mixin.


This comment no longer makes sense in this new context. It previously was in a "processing an extend impl" function, where extend impl would make sense in both a class and mixin scope.

josh11b · 2025-11-26T19:48:58Z

toolchain/check/facet_type.cpp

+      [&](const SemIR::FacetTypeInfo::RewriteConstraint& rewrite) {
+        auto access = context.insts().GetAs<SemIR::ImplWitnessAccess>(
+            GetImplWitnessAccessWithoutSubstitution(context, rewrite.lhs_id));
+        return WitnessQueryMatchesInterface(context, access.witness_id,
+                                            interface_to_witness);


Comment using words to describe what is going on here? Maybe something like:

Suggested change

[&](const SemIR::FacetTypeInfo::RewriteConstraint& rewrite) {

auto access = context.insts().GetAs<SemIR::ImplWitnessAccess>(

GetImplWitnessAccessWithoutSubstitution(context, rewrite.lhs_id));

return WitnessQueryMatchesInterface(context, access.witness_id,

interface_to_witness);

// A rewrite constraint requires `interface_to_witness` to have a witness

// table if its left-hand side names a member of that interface.

[&](const SemIR::FacetTypeInfo::RewriteConstraint& rewrite) {

auto access = context.insts().GetAs<SemIR::ImplWitnessAccess>(

GetImplWitnessAccessWithoutSubstitution(context, rewrite.lhs_id));

return WitnessQueryMatchesInterface(context, access.witness_id,

interface_to_witness);

Added this:

// An iterator over the rewrite_constraints where the LHS of the rewrite names // a member of the `interface_to_witness`. This filters out rewrites into // other interfaces, as they do not set values in the witness table.

toolchain/check/facet_type.cpp

josh11b · 2025-11-26T19:52:59Z

toolchain/check/handle_impl.cpp

+static auto GetImplDefaultSelfType(Context& context,
+                                   const ClassScope& class_scope)
+    -> SemIR::TypeId {
+  // TODO: This is also valid in a mixin.


This comment no longer makes sense in the context of a class scope. It was previously in a "maybe class scope" context.

toolchain/check/handle_impl.cpp

josh11b · 2025-11-26T20:21:51Z

toolchain/check/handle_impl.cpp

  // TODO: Revisit this once #3714 is resolved.
-  AddNameToLookup(context, SemIR::NameId::SelfType, self_type_inst_id);
+  AddNameToLookup(context, SemIR::NameId::SelfType, self_type.inst_id);
+  context.node_stack().Push(node_id, self_type.inst_id);


I don't see where this is popped.

It's popped in BuildImplDecl. The Push here is not new, it was on L60 before, but I moved it to be the last thing in the function since that is how they normally appear and helps me follow the push/pops.

toolchain/check/handle_impl.cpp

toolchain/check/name_scope.cpp

toolchain/check/impl.cpp

josh11b · 2025-11-26T21:46:35Z

toolchain/check/impl.cpp

+  }
+
+  if (is_definition && impl.witness_id != SemIR::ErrorInst::InstId) {
+    if (!RequireCompleteFacetTypeForImplDefinition(


I'm surprised to see this new call to RequireCompleteFacetTypeForImplDefinition. It still seems to be called from ImplWitnessStartDefinition.

Argh yeah that was how this whole PR started was to combine things, and I got lost (sorry it was hard to review, it was also hard to write and took me 3 tries...).

Now at the end of this PR I finally do understand exactly what was needed to get to a single RequireCompleteType instruction. It must be part of the definition, because in the case of a redeclaration, all generic decls must match, so we can't insert the RequireCompleteType instruction into the definition's declaration. So I have moved it appropriately into only ImplWitnessStartDefinition.

The result is that the declaration always makes a placeholder witness table if there's no rewrites, which gets resized in the definition. This eliminates a whole bunch of plumbing of is_definition around, further simplifying logic.

The tests churn a bunch with instruction orders changing, but we can see the RequireCompleteType inst moves to be consistently in the impl's definition.

danakj

Thanks, PTAL

toolchain/check/name_scope.cpp

danakj · 2025-11-26T22:07:30Z

toolchain/check/name_scope.cpp

+  if (!class_decl) {
+    return std::nullopt;
+  }
+  // TODO: This is also valid in a mixin.


danakj · 2025-11-26T22:11:06Z

toolchain/check/facet_type.cpp

+      [&](const SemIR::FacetTypeInfo::RewriteConstraint& rewrite) {
+        auto access = context.insts().GetAs<SemIR::ImplWitnessAccess>(
+            GetImplWitnessAccessWithoutSubstitution(context, rewrite.lhs_id));
+        return WitnessQueryMatchesInterface(context, access.witness_id,
+                                            interface_to_witness);


Added this:

// An iterator over the rewrite_constraints where the LHS of the rewrite names // a member of the `interface_to_witness`. This filters out rewrites into // other interfaces, as they do not set values in the witness table.

toolchain/check/facet_type.cpp

danakj · 2025-11-26T22:12:27Z

toolchain/check/handle_impl.cpp

+static auto GetImplDefaultSelfType(Context& context,
+                                   const ClassScope& class_scope)
+    -> SemIR::TypeId {
+  // TODO: This is also valid in a mixin.


toolchain/check/handle_impl.cpp

danakj · 2025-11-26T22:17:59Z

toolchain/check/handle_impl.cpp

  // TODO: Revisit this once #3714 is resolved.
-  AddNameToLookup(context, SemIR::NameId::SelfType, self_type_inst_id);
+  AddNameToLookup(context, SemIR::NameId::SelfType, self_type.inst_id);
+  context.node_stack().Push(node_id, self_type.inst_id);


It's popped in BuildImplDecl. The Push here is not new, it was on L60 before, but I moved it to be the last thing in the function since that is how they normally appear and helps me follow the push/pops.

toolchain/check/handle_impl.cpp

toolchain/check/impl.cpp

danakj · 2025-11-26T22:45:59Z

toolchain/check/impl.cpp

+  }
+
+  if (is_definition && impl.witness_id != SemIR::ErrorInst::InstId) {
+    if (!RequireCompleteFacetTypeForImplDefinition(


Argh yeah that was how this whole PR started was to combine things, and I got lost (sorry it was hard to review, it was also hard to write and took me 3 tries...).

Now at the end of this PR I finally do understand exactly what was needed to get to a single RequireCompleteType instruction. It must be part of the definition, because in the case of a redeclaration, all generic decls must match, so we can't insert the RequireCompleteType instruction into the definition's declaration. So I have moved it appropriately into only ImplWitnessStartDefinition.

The result is that the declaration always makes a placeholder witness table if there's no rewrites, which gets resized in the definition. This eliminates a whole bunch of plumbing of is_definition around, further simplifying logic.

The tests churn a bunch with instruction orders changing, but we can see the RequireCompleteType inst moves to be consistently in the impl's definition.

danakj requested a review from a team as a code owner November 21, 2025 18:32

danakj requested review from josh11b and removed request for a team November 21, 2025 18:32

github-actions bot added the toolchain label Nov 21, 2025

danakj mentioned this pull request Nov 21, 2025

Type completion of facet types is separate from Identifying #6385

Merged

danakj added 4 commits November 24, 2025 10:22

less-completing-impl-as

e204482

more

223c0e0

fix-extra-diag

8868675

early-out

181c03c

danakj force-pushed the less-completing-impl-as branch from c713931 to 181c03c Compare November 24, 2025 15:23

danakj requested a review from zygoloid November 26, 2025 16:30

josh11b reviewed Nov 26, 2025

View reviewed changes

danakj added 2 commits November 26, 2025 17:45

review

4512317

single-require-complete

ba790ef

danakj commented Nov 26, 2025

View reviewed changes

danakj requested a review from josh11b November 26, 2025 23:08

Merge remote-tracking branch 'origin/trunk' into less-completing-impl-as

fbcfcdf

Refactor Impl construction to clarify and reduce extraneous work #6420

Are you sure you want to change the base?

Refactor Impl construction to clarify and reduce extraneous work #6420

Conversation

danakj commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danakj commented Nov 21, 2025

Uh oh!

danakj commented Nov 24, 2025

Uh oh!

josh11b left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danakj Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danakj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danakj Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

danakj commented Nov 21, 2025 •

edited

Loading

danakj Nov 26, 2025 •

edited

Loading

danakj Nov 26, 2025 •

edited

Loading