Type completion of facet types is separate from Identifying #6385

danakj · 2025-11-17T18:47:36Z

Identifying a facet type is an operation on a pair of (self type, facet type). It substitutes that self in as the Self of any require declarations in order to form the set of (self type, SpecificInterface) pairs that constitute the requirements of the IdentifiedFacetType. Currently we don't pass around any self type, and assume all require declarations are written against Self but this will change in the future.

By contrast, type completion is done in the abstract and does not form specifics for the require declarations. The purpose of type completion is to enumerate the scopes where name lookup can occur and ensure they are completed.

With this change, type completion is:

No longer built on top of identification for facet types.
Recursively ensures all extend scopes are complete since name lookup can find symbols in them.

We add some test cases that demonstrate consistency between a resolving the specific of a generic class, and a generic interface/constraint, both used in a type position. In all cases, an invalid specific is not materialized for the type completion when the specific's arguments are used in a non-extend context. But they specific is materialized and checked for type completion when in an extend context (extend impl or extend require).

Type completion itself does not need to recurse into named constraints or interfaces as the extend require declarations require the type to be complete immediately, just as for extend impl in a class.

We had a test (fail_incomplete_where.carbon) with impl as J where .Self impls K and J is incomplete, which used to be diagnosed but no longer is, because we don't require non-extend interfaces to be complete in type completion, nor in identification. The test was trying to test the presence of rewrite constraints though, which it didn't even use. So we remove the diagnostic that we can't hit anymore and replaced it with a TODO, and add a test that should reach that TODO once qualified rewrite constraints work.

danakj · 2025-11-17T18:50:18Z

@josh11b This comes from discussion with Richard on Friday, and we think may extend (ha ha) a little on what was proposed in https://github.com/carbon-language/carbon-lang/blob/trunk/proposals/p5168.md#proposed-rules since those rules do not mention extend.

Our discussion notes are here and I will be following up to introduce a self input for the identification of a facet type, so that we can resolve specifics with a real self for identification. For type completion it is left abstract. This difference doesn't exactly show up too much yet but will be critical for require T as U instead of require Self... as we will need to replace the T with some external self via the specific.

danakj · 2025-11-17T20:31:26Z

This needs a rebase to get tests happy, which I will get to later.

josh11b

This PR is fine, but it seems like we should update the official rules with a new proposal.

danakj · 2025-11-18T23:10:38Z

This PR is fine, but it seems like we should update the official rules with a new proposal.

I had tried to get rid of the recursion onto the facet types in require decls, as we check them for completeness where they are written. However when the interface is generic, the facet type can contain generic bindings in it, and those may have been OK as symbolics, but produce an error with a given specific. We have tests for these included in the PR, which removing the recursion broke.

So I have re-introduced the recursion (via worklist) on the specific facet type in an extend require decl, in order to:

Complete a specific for that facet type, in order to find errors.
Then look for more extern require decls in the interfaces/constraints that it names.

We only need to do this step of forming specifics and recursing into the extend require decls if the containing interface/constraint has a generic id, otherwise there are no generic bindings to apply a specific to.

danakj · 2025-11-19T15:18:38Z

We had a test with impl as J where .Self impls K and J is incomplete, which used to be diagnosed but no longer is, because we don't require non-extend interfaces to be complete in type completion, nor in identification. The test was trying to test the presence of rewrite constraints though, which it didn't even use. So I have removed the diagnostic that we can't hit anymore and replaced it with a TODO, and added a test that should reach that TODO once qualified rewrite constraints work.

danakj · 2025-11-19T17:04:20Z

The last commit now avoids a UAF which is properly addressed by #6399. I will revert that commit when the fix lands.

toolchain/check/testdata/generic/extend_type_completion.carbon

toolchain/check/testdata/impl/fail_undefined_interface.carbon

danakj

PTAL

toolchain/check/testdata/generic/extend_type_completion.carbon

toolchain/check/testdata/impl/fail_undefined_interface.carbon

zygoloid · 2025-11-19T22:09:21Z

toolchain/check/testdata/impl/impl_as_named_constraint.carbon

-// Requries B identified.
-impl () as B;


Do we have a test for

constraint B; impl () as B;

somewhere? (Which should be an error because B is not identified.)

We do here:

carbon-lang/toolchain/check/testdata/facet/fail_incomplete.carbon

Line 62 in 6b1ef75

impl C as X;

But I have now moved tests around to consolidate things better in impl/incomplete.carbon.

toolchain/check/impl_lookup.cpp

zygoloid · 2025-11-19T22:21:25Z

toolchain/check/type_completion.cpp

+    for (auto extends : facet_type_info.extend_constraints) {
+      auto interface_id = extends.interface_id;
+      const auto& interface = context.interfaces().Get(interface_id);
+      if (!interface.is_complete()) {
+        if (diagnoser) {
+          auto builder = diagnoser();
+          NoteIncompleteInterface(context, interface_id, builder);
+          builder.Emit();
+        }
+        return false;
+      }
+      if (interface.generic_id.has_value()) {
+        specifics.push_back(
+            {extends.specific_id, interface.require_impls_block_id});
+      }
+    }
+
+    for (auto extends : facet_type_info.extend_named_constraints) {
+      auto named_constraint_id = extends.named_constraint_id;
+      const auto& constraint =
+          context.named_constraints().Get(named_constraint_id);
+      if (!constraint.is_complete()) {
+        if (diagnoser) {
+          auto builder = diagnoser();
+          NoteIncompleteNamedConstraint(context, named_constraint_id, builder);
+          builder.Emit();
+        }
+        return false;
+      }
+      if (constraint.generic_id.has_value()) {
+        specifics.push_back(
+            {extends.specific_id, constraint.require_impls_block_id});
+      }
+    }


Would be nice if we could find some way to avoid this duplication, maybe add a TODO?

I'm not sure. This is emblematic of the duplication between interfaces and named constraints across the toolchain. Since they have different types, with different field names for ids, etc, we end up with a lot of very similar code. I have tried elsewhere to use field pointers and templates but it gets really unreadable really quickly. I don't have any great ideas for how I would address that TODO right now.

zygoloid · 2025-11-19T22:33:30Z

toolchain/check/type_completion.cpp

+    auto requires_specifics =
+        llvm::ArrayRef(specifics).drop_front(specifics_done);
+    for (auto [specific_id, requires_block_id] : requires_specifics) {
+      for (auto require_impls_id :
+           context.require_impls_blocks().Get(requires_block_id)) {
+        const auto& require = context.require_impls().Get(require_impls_id);
+        if (require.extend_self) {
+          auto require_specific_id = MakeCopyOfSpecificAndAppendSelf(
+              context, loc_id, specific_id, require.generic_id,
+              GetRequireImplsSpecificSelf(context, require));
+          // Construct the specific facet type. If None is returned, it means an
+          // error was diagnosed.
+          auto facet_type_id = GetFacetTypeInSpecific(
+              context, require_specific_id, require.facet_type_inst_id);
+          if (!facet_type_id.has_value()) {
+            return false;
+          }
+          work.push_back(facet_type_id);
+        }
+      }


I think we may be able to avoid this:

In handle_require.cpp, move the RequireCompleteType call for an extend require impls declaration later, to after we finish the generic for the RequireImplsDecl.

That way, if the enclosing context is itself generic, then we'll create a require_complete_type instruction in the generic for the interface / named constraint, which will cause the ResolveSpecificDefinition below to recursively require the extended type to be complete. (That's how we get recursive completeness requirements for extend in classes.)

Trivially making this change did not work:

CHECK failure at toolchain/sem_ir/generic.cpp:96: specific_ir.generics().GetSelfSpecific( specific_ir.specifics().Get(specific_id).generic_id) == specific_id: Queried generic_inst_in_decl0 in specific5800002B for {kind: NamedConstraintDecl, arg0: constraint58000000, arg1: inst_block5800001B, type: type(inst58000041)} before it was resolved.

This looks like when I tried to resolve an inner specific facet type before resolving the containing constraint's specific. I think this strategy maybe just works for constraint->require but does not work for constraint->require->constraint->require as there's nothing crossing that middle require->constraint boundary.

I can try understand this a bit better tomorrow.

Ok I found one issue.

ResolveSpecificDeclForInst() in eval was not resolving specific decl blocks for named constraints in a facet type after applying a specific.

But fixing that turned up a crash in import instead (that I can reproduce on interfaces too without that fix).

For the record this crash was fixed by Jon in #6409.

Alright, this is done. I apologize cuz I have rebased this a few times to make it work since HEAD had fixes I needed, and mixing commits with merges makes for real confusion (for me?).

This reverts commit 8c5e573.

danakj · 2025-11-20T22:32:35Z

toolchain/check/facet_type.cpp

+    context.TODO(loc_id,
+                 "declaration of impl as incomplete interface with a rewrite "
+                 "constraint");


I am quite convinced this case should no longer ever happen so I changed it into a CHECK with a TODO to remove the parameter. I tried to organize the type completion of the facet type in impl as a bit better since it occurs in two different places with some loose coordination right now, but it's very tricky. I will try again elsewhere.

danakj requested a review from a team as a code owner November 17, 2025 18:47

danakj requested review from chandlerc and josh11b and removed request for a team November 17, 2025 18:47

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

danakj removed the request for review from chandlerc November 17, 2025 18:47

danakj force-pushed the complete-not-identify branch from c7e3481 to eafaeb1 Compare November 17, 2025 20:33

josh11b approved these changes Nov 18, 2025

View reviewed changes

danakj force-pushed the complete-not-identify branch 3 times, most recently from 483b33d to 90e932d Compare November 19, 2025 14:22

danakj requested a review from josh11b November 19, 2025 15:20

danakj force-pushed the complete-not-identify branch from 6b368f8 to 7aa776d Compare November 19, 2025 20:14

josh11b reviewed Nov 19, 2025

View reviewed changes

toolchain/check/testdata/generic/extend_type_completion.carbon Outdated Show resolved Hide resolved

toolchain/check/testdata/impl/fail_undefined_interface.carbon Outdated Show resolved Hide resolved

danakj commented Nov 19, 2025

View reviewed changes

toolchain/check/testdata/generic/extend_type_completion.carbon Outdated Show resolved Hide resolved

toolchain/check/testdata/impl/fail_undefined_interface.carbon Outdated Show resolved Hide resolved

danakj requested review from josh11b and zygoloid and removed request for josh11b November 19, 2025 21:20

zygoloid reviewed Nov 19, 2025

View reviewed changes

danakj force-pushed the complete-not-identify branch from 6a5962d to beb7d87 Compare November 19, 2025 23:25

danakj added 5 commits November 20, 2025 17:13

complete-not-identify

98a72b9

touchup

1a64cdc

MakeCopyOfSpecificAndAppendSelf

e271440

bit-simpler

ac0469b

whitespace

acd9333

danakj added 21 commits November 20, 2025 17:13

complete-at-require-decl

b8e5c89

no-recurse-needed

b92f9b1

recurse-on-facets

8219586

autoupdate

c183562

comment

97c2a3c

rewrites-complete

bb93fd5

autoupdate

92e0bc1

avoid-uaf

47b1728

test-and-fix-multi-level-specific-resolution

c312a16

one-less-malloc

fe9a842

add-comment

aa2fcc0

extends

4fe0365

no-k

a35442d

fix-test

56886b7

Revert "avoid-uaf"

cc27be4

This reverts commit 8c5e573.

move-tests

9305cca

try-move-requirecompletetype

05af854

works-but-import-crash

a18bbae

wip

3939132

works

ec8e2d4

yay

e1fc972

danakj force-pushed the complete-not-identify branch from beb7d87 to e1fc972 Compare November 20, 2025 22:18

danakj requested a review from zygoloid November 20, 2025 22:27

remove-todo-diagnostic

00b9dc0

danakj commented Nov 20, 2025

View reviewed changes

Type completion of facet types is separate from Identifying #6385

Are you sure you want to change the base?

Type completion of facet types is separate from Identifying #6385

Conversation

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

Uh oh!

Uh oh!

danakj commented Nov 17, 2025

Uh oh!

danakj commented Nov 17, 2025

Uh oh!

josh11b left a comment

Choose a reason for hiding this comment

Uh oh!

danakj commented Nov 18, 2025

Uh oh!

danakj commented Nov 19, 2025

Uh oh!

danakj commented Nov 19, 2025

Uh oh!

Uh oh!

Uh oh!

danakj left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danakj Nov 20, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

danakj commented Nov 17, 2025 •

edited

Loading

danakj Nov 20, 2025 •

edited

Loading