Implement support for copying C++ classes. #6434

zygoloid · 2025-11-25T23:16:14Z

When performing impl lookup for Core.Copy for a C++ class type, look for a copy constructor. If we find one, synthesize an impl witness that calls the constructor.

This adds initial support for impl lookup to delegate to the C++ interop logic for queries involving C++ types. For now, we don't implement the rules from #6166 that compare a synthesized type structure for the C++ impl against the best Carbon type structure, but the framework for building that support is established here.

Currently there is no caching of the lookup here, and we build unique ImplWitnessTables for each lookup, which leads to each impl lookup producing a distinct facet value. This results in some errors in generic contexts; this will be addressed in follow-up changes. This PR aims only to support the non-generic case.

When performing impl lookup for `Core.Copy` for a C++ class type, look for a copy constructor. If we find one, synthesize an impl witness that calls the constructor. This adds initial support for impl lookup to delegate to the C++ interop logic for queries involving C++ types. For now, we don't implement the rules from carbon-language#6166 that compare a synthesized type structure for the C++ impl against the best Carbon type structure, but the framework for building that support is established here. Currently there is no caching of the lookup here, and we build unique `ImplWitnessTable`s for each lookup, which leads to each impl lookup producing a distinct facet value. This results in some errors in generic contexts; this will be addressed in follow-up changes. This PR aims only to support the non-generic case.

toolchain/check/cpp/impl_lookup.cpp

danakj · 2025-11-25T23:31:59Z

toolchain/check/cpp/impl_lookup.cpp

+  // represent a synthesized witness.
+  auto witness_table_inst_id = AddInst<SemIR::ImplWitnessTable>(
+      context, loc_id,
+      {.elements_id = witness_table_id, .impl_id = SemIR::ImplId::None});


I would really prefer we introduce a special ImplId::Cpp for this, if that seems like it would work?

Of course, then code has to check for it before looking in ImplStore (and in printing). But it would make the semir more clear I think?

Alternatively, I was thinking about adding a SynthesizedWitnessTable instead and stopping using ImplWitnessTable here. Part of the idea would be that we could give it an Always instead of AlwaysUnique constant kind so it gets properly deduplicated. (There are circularity problems if the interface declares associated constants that need to be substituted into the signatures of later functions; I'm not yet sure if this is viable.) What do you think?

I'm fine with seeing how far we can take that, yeah. (I am not sure I see what problem will come up with associated constants.) But Always can't be symbolic and depend on generic params right?

Always can be symbolic; it doesn't imply a concrete constant value, just a non-runtime one.

Adding a new kind of witness table would be a fairly significant change I think; what do you want done in this PR? I'm happy to add the special ImplId for now if you would like to avoid a None here.

If we're going to follow on with another inst, then a TODO here is fine.

danakj · 2025-11-25T23:39:12Z

toolchain/check/cpp/impl_lookup.cpp

+                   SemIR::TypeId self_type_id,
+                   SemIR::SpecificInterface specific_interface,
+                   const TypeStructure* best_impl_type_structure,
+                   SemIR::LocId best_impl_loc_id) -> EvalImplLookupResult {


I think I would prefer if all of these functions returned SemIR::InstBlockIdOrError, which is the output of the more-public LookupImplWitness(). The eval stuff is a bit of an implementation detail, only in the header to make it visible to eval. Is there any reason why we couldn't do that?

I had been thinking there might be times when we would return a non-final success result here, when we can see there's a C++ operator that would match but we can't instantiate it yet. But maybe that's just too theoretical, and it's definitely outside what's been designed. I'll change this as suggested, and we can change it back if needed.

That might still be okay, as the InstBlock would contain a symbolic witness (a LookupImplWitness or something else) instead of a concrete one (an ImplWitness). Unless you want to avoid making any witness instruction for that case.

I was thinking it'd be better to avoid making a witness in that case, especially since the caller would just throw it away (and we wouldn't really have values to put in it, since we can't form symbolic "instantiations" of C++ templates).

InstBlockIdOrError didn't seem to work, since we have a witness not an InstBlockId, but I've switched to returning an InstId for the witness instead.

danakj · 2025-11-25T23:56:51Z

toolchain/check/impl_lookup.cpp

+  return candidates;
+}
+
+// Given a value that is either a type or a non-type facet, returns the


"non-type facet" reads to me at first like a symbolic non-type, but I think this is saying a "type or a facet value", so could we write it that way? That's how I worded similar in other places.

Hm, this is a bit hard to express unambiguously. To me, "type or facet value" is redundant, because a type is a facet, but simply "facet value" would be confusing because a type isn't a FacetValue and its type isn't a FacetType even though it IsFacetType. :-/

We use "non-type facet" to describe a facet that is not a type in, for example, https://docs.carbon-lang.dev/docs/design/expressions/member_access.html, so it's not unprecedented. But maybe something more toolchain-oriented would work. Let me try something...

heh.. yeah. Well FWIW when I use "facet value" as such, I mean a value of type FacetType, meaning a symbolic type, rather than a concrete type (of type type).

danakj · 2025-11-26T00:02:14Z

toolchain/check/impl_lookup.cpp

+static auto GetFacetAsType(Context& context, SemIR::LocId loc_id,
+                           SemIR::ConstantId facet_or_type_const_id)
+    -> SemIR::TypeId {
+  auto facet_or_type_id =
+      context.constant_values().GetInstId(facet_or_type_const_id);
+  auto type_type_id = context.insts().Get(facet_or_type_id).type_id();
+  CARBON_CHECK(context.types().IsFacetType(type_type_id) ||
+               type_type_id == SemIR::ErrorInst::TypeId);
+
+  if (context.types().Is<SemIR::FacetType>(type_type_id)) {
+    // It's a facet; access its type.
+    facet_or_type_id = GetOrAddInst<SemIR::FacetAccessType>(
+        context, loc_id,
+        {.type_id = SemIR::TypeType::TypeId,
+         .facet_value_inst_id = facet_or_type_id});
+  }
+  return context.types().GetTypeIdForTypeInstId(facet_or_type_id);
 }


I guess this is a cheaper version of calling ExprAsType, with CHECKs involved, right? I had thought of replacing some other places that construct FacetAccessType with ExprAsType. Like

https://github.com/carbon-language/carbon-lang/blob/a179bd461b4a07bf2defe91e68f1bb9edfa60551/toolchain/check/handle_require.cpp#L74C1-L77C1

Maybe this should go live beside ExprAsType so we can reuse it?

This is converting a ConstantId, not an InstId, so I don't think it's really at quite the same level as ExprAsType. It might make sense to move this to facet_type.cpp, though?

Alternatively, we could change the LookupImplWitness instruction so that its query_self_inst_id is a TypeInstId rather than a type-or-non-type-facet instruction, and we wouldn't need to do this conversion within the lookup. That'd also make it a bit more obvious what the canonical form for a symbolic LookupImplWitness instruction is. Would we lose anything by doing that? I think the only thing that would affect is a lookup whose self is a symbolic binding or associated type, where we'd add an extra FacetAccessType around the self.

What do you think? Move this facet_type.cpp, or try to avoid the need for it entirely?

Just to throw in a third possibility: EvalLookupSingleImplWitness wants to work with the canonical self, which it does by getting the ConstantId, but it could just as easily do GetConstantInstId and work with an InstId throughout, for better or for worse. That would make this more like ExprAsType, and avoid the first step inside where it moves from ConstantId to InstId. But I think we do expect to work with a ConstantId more places than I originally was thinking, such as around deduce. So maybe not.

I used to be really thrown off by extraneous FacetAccessType getting thrown around when we could avoid it. It made things harder to understand for me originally. So 6 months ago I would have not wanted that I think. But now I think that sounds like a good idea. Probably something to do in another PR?

For what it's worth: GetConstantInstId would produce an instruction without a location, which I think isn't a good thing to pass into ExprAsType, because it expects an expression-as-written-in-source, might mutate the source if it's an initializing expression, and might point diagnostics at the location of the source expression. In general I think we should be a bit careful what we do with InstIds we get from GetConstantInstId since they don't have properties that code dealing with expressions might expect (such as having a location).

Will look into changing the self of LookupImplWitness to being a type as a separate PR.

danakj · 2025-11-26T00:04:23Z

toolchain/check/impl_lookup.cpp

  }

+  if (query_is_concrete && candidates.consider_cpp_candidates) {
+    CARBON_CHECK(!self_facet_provides_witness);


Can we leave a comment explaining why this is true?

Reworked a little to avoid the check.

I like the rework there, thanks.

danakj · 2025-11-26T00:10:48Z

toolchain/check/impl_lookup.cpp


+  if (query_is_concrete && candidates.consider_cpp_candidates) {
+    CARBON_CHECK(!self_facet_provides_witness);
+    return LookupCppImpl(context, loc_id,


Suggested change

return LookupCppImpl(context, loc_id,

// No Carbon candidates were found for the concrete query. Try find a C++ one, without citing

// a Carbon candidate to compare with.

return LookupCppImpl(context, loc_id,

Added a similar comment.

danakj · 2025-11-26T00:11:30Z

toolchain/check/impl_lookup.cpp

      }
+
+      if (query_is_concrete && candidates.consider_cpp_candidates) {
+        auto cpp_result = LookupCppImpl(


Suggested change

auto cpp_result = LookupCppImpl(

// We prefer a C++ candidate if it's a better match than the Carbon candidate we have found.

auto cpp_result = LookupCppImpl(

Done (though edited in light of other changes in this area).

danakj · 2025-11-26T00:12:38Z

toolchain/sem_ir/inst_fingerprinter.cpp


  auto Add(ImplId impl_id) -> void {
+    if (!impl_id.has_value()) {
+      AddInvalid();


I found this a bit surprising? I mean that a C++ witness isn't invalid, I'd expect that it fingerprints reasonably. Maybe AddInvalid isn't as problematic as I am imagining?

I think we just forgot to rename this when we renamed FooId::Invalid to FooId::None.

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

toolchain/check/cpp/impl_lookup.cpp

Co-authored-by: Carbon Infra Bot <[email protected]>

danakj

LGTM

It is introduced in #6434

zygoloid requested a review from a team as a code owner November 25, 2025 23:16

zygoloid requested review from danakj and removed request for a team November 25, 2025 23:16

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

danakj reviewed Nov 26, 2025

View reviewed changes

zygoloid and others added 2 commits November 26, 2025 11:29

Add const

48f494b

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

Add const

405a01a

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

CarbonInfraBot reviewed Nov 26, 2025

View reviewed changes

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

zygoloid and others added 3 commits November 26, 2025 12:41

clang-format suggested edit

0e344a5

Co-authored-by: Carbon Infra Bot <[email protected]>

Address some review comments.

508d62c

Use InstId instead of EvalImplLookupResult.

7ed2ec3

zygoloid requested a review from danakj November 26, 2025 21:06

danakj approved these changes Nov 26, 2025

View reviewed changes

danakj mentioned this pull request Nov 26, 2025

Use the new IsFacetTypeOrError function #6438

Merged

zygoloid added 3 commits December 2, 2025 02:01

Add TODO

aa7765f

Merge branch 'trunk' into toolchain-interop-impl-lookup

fcf0fb6

Autoupdate

c0e9244

zygoloid enabled auto-merge December 2, 2025 02:14

zygoloid added this pull request to the merge queue Dec 2, 2025

Merged via the queue into carbon-language:trunk with commit 372f632 Dec 2, 2025
8 checks passed

zygoloid deleted the toolchain-interop-impl-lookup branch December 2, 2025 03:29

github-merge-queue bot pushed a commit that referenced this pull request Dec 2, 2025

Use the new IsFacetTypeOrError function (#6438)

e5c94b1

It is introduced in #6434

	auto cpp_result = LookupCppImpl(
	// We prefer a C++ candidate if it's a better match than the Carbon candidate we have found.
	auto cpp_result = LookupCppImpl(

Implement support for copying C++ classes. #6434

Implement support for copying C++ classes. #6434

Uh oh!

Conversation

zygoloid commented Nov 25, 2025

Uh oh!

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!

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danakj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants