Forward `impl` declarations of incomplete facet types #5219

josh11b · 2025-03-29T03:18:23Z

Implements some of the changes from proposal #5168.

The data structure for complete facet types has been repurposed for identified facet types. Identified facet types are now a concept in the toolchain, but without named constraint support they are not substantially different from incomplete facet types.
Identified facet types keep the list of required specific interfaces in sorted order, for efficiency improvements in impl lookup. Found another way to identify the interface to impl (or number of impls if not 1).
Forward impl declarations of identified but incomplete facet types are allowed unless the facet type has rewrites. An incomplete facet type with rewrites is already either an error or has more than one interface and so can't be implemented, so this case can't be exercised very well yet.
Forward impl declarations of interface without rewrites use a placeholder inst block for the witness.
Changed some machinery to use RequireIdentifiedFacetType to access the interfaces of the facet type so we only need to add support for expanding named constraints into interfaces in one place.

toolchain/check/eval_inst.cpp

toolchain/check/facet_type.cpp

danakj · 2025-03-31T16:38:41Z

toolchain/check/facet_type.cpp

+  // this.
+  CARBON_CHECK(is_definition);
+  CARBON_DIAGNOSTIC(ImplAsIncompleteFacetTypeDefinition, Error,
+                    "impl as incomplete facet type {0} definition",


i have trouble reading this grammatically, so suggesting something else:

Suggested change

"impl as incomplete facet type {0} definition",

"`impl as` target has facet type {0} that is not yet defined",

I was trying to convey that impl definitions specifically require the facet type to be complete (now unlike impl declarations).

Maybe something like definition of impl as incomplete facet type {0}?

danakj · 2025-03-31T16:40:24Z

toolchain/check/facet_type.cpp

  // TODO: Finish facet type resolution. This code currently only handles
  // rewrite constraints that set associated constants to a concrete value.
  // Need logic to topologically sort rewrites to respect dependencies, and
  // afterwards reject duplicates that are not identical.

+  auto facet_type_id =
+      context.types().GetTypeIdForTypeInstId(facet_type_inst_id);
+  CARBON_CHECK(facet_type_id != SemIR::ErrorInst::SingletonTypeId);


Generally we want to pass errors along through the system, how come we want a CHECK instead of a return here?

Because the case should be unreachable here.

danakj · 2025-03-31T16:42:32Z

toolchain/check/facet_type.cpp

+        context, witness_loc_id,
+        {.type_id =
+             GetSingletonType(context, SemIR::WitnessType::SingletonInstId),
+         .elements_id = context.inst_blocks().AddPlaceholder(),


I wonder if we should have a special InstBlockId for this to differentiate from an empty block?

These ids should not be distinguished. This is a block that will be filled in later without changing its id. I've added a comment.

danakj · 2025-03-31T17:12:44Z

toolchain/check/testdata/impl/min_prelude/forward_declarations.carbon

+  // require Self impls Y;
+}
+
+// Classes must be defined before being used in a function definition.


There's a test for this somewhere?

I'm not sure, but it is not the topic of this PR. This comment I believe is copied from the proposal.

toolchain/check/testdata/impl/min_prelude/forward_declarations.carbon

danakj · 2025-03-31T17:20:07Z

toolchain/check/type_completion.h

+// set of interfaces the facet type requires is known. Since named constraints
+// are not yet supported, this currently never fails. Eventually this function
+// will be passed a diagnoser for facet types that use some incomplete named
+// constraint, and return `None` in that case.


Mention that this ensures the FacetType is present in identified_facet_types() when it returns non-None?

danakj · 2025-03-31T17:22:44Z

toolchain/sem_ir/facet_type_info.h

+  // the facet type to the right of an `impl`...`as`, or `None` if no such
+  // single interface.
+  InterfaceId interface_id;
+  union {


Can we use a std::variant so that we get checks that verify we don't access the wrong one and make UB?

I think that would mean extra storage for the discriminator.

danakj · 2025-03-31T17:29:39Z

toolchain/sem_ir/facet_type_info.h

+  // The single interface from `required_interfaces` to implement if this is
+  // the facet type to the right of an `impl`...`as`, or `None` if no such
+  // single interface.
+  InterfaceId interface_id;


These fields are duplicating the first element of required_interfaces, but adding a bunch of complexity for using the IntentifiedFacetType.

I was wondering if a method would suit our purposes just as well, without causing us to use variants/unions and so on?

Something like this?

auto is_impl_as_target() -> bool { return required_interfaces.size() == 1; } auto impl_as_target_interface( ) -> SpecificInterface { if (is_impl_as_target()) { return required_interfaces.front(); } else { return None; } }

Or do you want to use these to hide the num_to_impl specifically when it's not used?

One of my goals for this PR is to have the identified interfaces be a sorted list of all the required interfaces, mixing together the ones to implement. I'm hoping to follow this up with a PR to support where .Self impls J, at which point the one to implement won't necessarily be first. Adding some accessors to hide this complexity and maintain the invariants is a good idea though.

Co-authored-by: Dana Jansens <[email protected]>

toolchain/check/eval_inst.cpp

toolchain/check/facet_type.cpp

…rve invariants

Co-authored-by: Dana Jansens <[email protected]>

josh11b · 2025-03-31T19:58:31Z

toolchain/check/facet_type.cpp

+  // this.
+  CARBON_CHECK(is_definition);
+  CARBON_DIAGNOSTIC(ImplAsIncompleteFacetTypeDefinition, Error,
+                    "impl as incomplete facet type {0} definition",


I was trying to convey that impl definitions specifically require the facet type to be complete (now unlike impl declarations).

josh11b · 2025-03-31T19:59:31Z

toolchain/check/facet_type.cpp

  // TODO: Finish facet type resolution. This code currently only handles
  // rewrite constraints that set associated constants to a concrete value.
  // Need logic to topologically sort rewrites to respect dependencies, and
  // afterwards reject duplicates that are not identical.

+  auto facet_type_id =
+      context.types().GetTypeIdForTypeInstId(facet_type_inst_id);
+  CARBON_CHECK(facet_type_id != SemIR::ErrorInst::SingletonTypeId);


Because the case should be unreachable here.

josh11b · 2025-03-31T20:03:05Z

toolchain/check/facet_type.cpp

+        context, witness_loc_id,
+        {.type_id =
+             GetSingletonType(context, SemIR::WitnessType::SingletonInstId),
+         .elements_id = context.inst_blocks().AddPlaceholder(),


These ids should not be distinguished. This is a block that will be filled in later without changing its id. I've added a comment.

josh11b · 2025-03-31T20:08:45Z

toolchain/check/handle_impl.cpp

-  }
-  const auto& complete = context.complete_facet_types().Get(complete_id);
-  if (complete.num_to_impl != 1) {
+  auto identified_id = RequireIdentifiedFacetType(context, *facet_type);


I'm happy to change the terminology, but that is really a topic for the proposal. I just haven't heard a clearer term. I feel like "identified" is less misleading than "fully declared" and I think users will have fewer preconceived ideas what "identified" means so it is more open for us to give it our own meaning.

josh11b · 2025-03-31T20:14:35Z

toolchain/check/impl.cpp

+  // required to be complete.
+  if (witness.elements_id != SemIR::InstBlockId::Empty &&
+      witness_block.empty()) {
+    if (!RequireCompleteFacetTypeForImplDefinition(


In the other case, it was already checked in InitialFacetTypeImplWitness.

josh11b · 2025-03-31T21:33:28Z

toolchain/check/handle_impl.cpp

-    impl_info.interface = *required_interface;
-  }
-
+  impl_info.interface = CheckConstraintIsInterface(context, impl_info);


Done. It required changing the signature of CheckConstraintIsInterface, but that wasn't a problem.

josh11b · 2025-03-31T22:17:44Z

toolchain/check/testdata/impl/fail_extend_partially_defined_interface.carbon

-    // CHECK:STDERR: fail_extend_partially_defined_interface.carbon:[[@LINE-5]]:1: note: interface is currently being defined [InterfaceIncompleteWithinDefinition]
-    // CHECK:STDERR: interface I {
-    // CHECK:STDERR: ^~~~~~~~~~~~~
-    // CHECK:STDERR:
    // CHECK:STDERR: fail_extend_partially_defined_interface.carbon:[[@LINE+4]]:5: error: impl declared but not defined [ImplMissingDefinition]


Well, I'm definitely missing code to enforce I is complete in this case. So that is now added. I don't know about your other question.

josh11b · 2025-03-31T22:51:48Z

toolchain/check/testdata/impl/min_prelude/forward_declarations.carbon

@@ -0,0 +1,2867 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM


I just realized we abbreviate "declarations", so I'll rename this file before merging.

josh11b · 2025-03-31T23:18:58Z

toolchain/check/testdata/impl/min_prelude/forward_declarations.carbon

+interface X;
+
+// Allowed to use incomplete interfaces in function declarations.
+fn F(U:! X);


josh11b · 2025-03-31T23:19:33Z

toolchain/check/testdata/impl/min_prelude/forward_declarations.carbon

+  // require Self impls Y;
+}
+
+// Classes must be defined before being used in a function definition.


I'm not sure, but it is not the topic of this PR. This comment I believe is copied from the proposal.

josh11b added 14 commits March 27, 2025 18:05

Checkpoint progress.

97d2abc

Checkpoint progress.

affbcf0

Checkpoint progress.

7df7ef6

Checkpoint progress.

cc54b69

Merge remote-tracking branch 'upstream/trunk' into impls

bf65e3f

Comment

28c3070

Checkpoint progress.

67654ca

Working

df39b1f

Address FIXME in type_completion

08f4499

Add todos and test

9380acd

Polish

5f60d2d

Merge remote-tracking branch 'upstream/trunk' into impls

46d979e

Forward-looking tests

69303fe

Example from proposal carbon-language#5168

9daedfc

github-actions bot added the toolchain label Mar 29, 2025

github-actions bot requested a review from danakj March 29, 2025 03:18

Fix clang-tidy

30be802

danakj reviewed Mar 31, 2025

View reviewed changes

Apply suggestions from code review

8d223f0

Co-authored-by: Dana Jansens <[email protected]>

CarbonInfraBot reviewed Mar 31, 2025

View reviewed changes

toolchain/check/eval_inst.cpp Outdated Show resolved Hide resolved

toolchain/check/facet_type.cpp Outdated Show resolved Hide resolved

josh11b and others added 7 commits March 31, 2025 20:28

Fix formatting

bb82c31

Make IdentifiedFacetType data private, provide accessors that prese…

e1e053d

…rve invariants

Changes from review suggestions

a3f1727

Require complete facet type after extend impl as

d34a5a4

Diagnostic is no longer reachable

0e198f6

Checkpoint progress.

3b032f6

Apply suggestions from code review

afa2fee

Co-authored-by: Dana Jansens <[email protected]>

This comment was marked as off-topic.

Sign in to view

Update test

7204ad3

josh11b commented Mar 31, 2025

View reviewed changes

Fix test

fe9ac49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Forward `impl` declarations of incomplete facet types #5219

Forward `impl` declarations of incomplete facet types #5219

josh11b commented Mar 29, 2025 •

edited

Loading

danakj Mar 31, 2025

josh11b Mar 31, 2025

zygoloid Mar 31, 2025

danakj Mar 31, 2025

josh11b Mar 31, 2025

danakj Mar 31, 2025

josh11b Mar 31, 2025

danakj Mar 31, 2025

josh11b Mar 31, 2025

danakj Mar 31, 2025

josh11b Mar 31, 2025

danakj Mar 31, 2025

josh11b Mar 31, 2025

danakj Mar 31, 2025

josh11b Mar 31, 2025

This comment was marked as off-topic.

josh11b Mar 31, 2025

josh11b Mar 31, 2025

josh11b Mar 31, 2025

josh11b Mar 31, 2025

josh11b Mar 31, 2025

josh11b Mar 31, 2025

josh11b Mar 31, 2025

josh11b Mar 31, 2025

josh11b Mar 31, 2025

josh11b Mar 31, 2025

	"impl as incomplete facet type {0} definition",
	"`impl as` target has facet type {0} that is not yet defined",

		@@ -0,0 +1,2867 @@
		// Part of the Carbon Language project, under the Apache License v2.0 with LLVM

Forward impl declarations of incomplete facet types #5219

Are you sure you want to change the base?

Forward impl declarations of incomplete facet types #5219

Conversation

josh11b commented Mar 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Forward `impl` declarations of incomplete facet types #5219

Forward `impl` declarations of incomplete facet types #5219

josh11b commented Mar 29, 2025 •

edited

Loading