Add builtin functions for destroy, with special requirements in facet types #6035

jonmeow · 2025-09-09T22:08:20Z

This is in support of a goal of changing the blanket destroy impl to use (roughly):

private fn CanAggregateDestroy() -> type = "type.can_aggregate_destroy";

// Handles aggregate type destruction.
impl forall [AggregateDestroyT:! CanAggregateDestroy()] AggregateDestroyT as Destroy {
  fn Op[addr self: Self*]() = "type.aggregate_destroy";
}

That isn't done here because there's still other issues that migrating raises. What this does do is add the builtin functions, and in particular, support to FacetTypeInfo to make CanAggregateDestroy work.

The "special requirement" approach in FacetTypeInfo allows us to support restricting a blanket impl under the current approach of impls. Maybe we'll find a cleaner approach that can work in the future, but this fits into the current model by propagating similar to other requirements. I'm using an enum mask because we have a number of similar things to add (e.g. copy, move) but I'm not sure we need a full vector.

A few alternatives considered were:

Supporting syntax more like where .Self impls TypeCanAggregateDestroy(.Self, SupportedInterface, UnsupportedInterface). I think it'd be a little cleaner, but requires better compile-time evaluation in order to assess the type of the call. Right now it's expected to be a FacetType too early to make this work, and I was concerned about pouring too much more time down this route.
Providing an actual interface, in particular doing name lookup back into Core. for an interface. This would've added name lookup overhead, and the question of whether an impl exists.
Generating an interface. This avoids the name lookup, but would still raise the question of whether an impl should also be generated. Work I've previously done generating interfaces for class destruction also feels complex to both write and understand (an unfortunate issue).
Still modeling as an ImplsConstraint, for example by defining a special InterfaceId::CanAggregateDestroy = -2 similar to what we do on other ids. I was hesitant because of how this expands the number of modes of InterfaceId, and things for consuming code to watch out for, for what feels like a relatively niche set of use-cases that are only interface-like.

… types

danakj · 2025-09-10T18:23:21Z

toolchain/check/testdata/builtins/type/aggregate_destroy.carbon

+  //@dump-sem-ir-end
+}
+
+// --- todo_fail_wrong_type.carbon


What is supposed to fail in here? Leave a TODO?

Added TODO below.

isn't it the exact same signature as the Op in the test above?

Above, Self is () or {}. In this test, Self is class C. Can't that be an error in the declaration, since it's not a blanket impl?

Oh I see, because C is not AggregateDestroy? Interesting, the actual interface is a builtin one (CanAggregateDestroy()). It's a bit of an extra step to also restrict which impls are allows to use the builtin function. Is the plan to require it to be in an impl which is a blanket impl and implementing the built-in interface (though this test is not)? Or would it make sense to just not allow it in any impl outside Core, or something?

Because C is not intended to be destructed through type.aggregate_destroy, yes. Note that CanAggregateDestroy isn't mentioned in the function:

impl C as DestroyLike { // TODO: This should fail because it has the wrong type. fn Op[addr self: Self*]() = "type.aggregate_destroy"; }

Is the plan to require it to be in an impl which is a blanket impl and implementing the built-in interface (though this test is not)?

Three ends where I think validation may be possible:

Uses addr self, not an arbitrary pointer ("// TODO: The argument should be addr self: Self*.")

Is a blanket impl of the right builtin constraint, and/or whatever type information it sees matches that.

If we can easily do validation when creating a specific, double-check there.

I think it should be possible to do more, but in particular addr self requires modifying ValidateSignature, or creating something different, because ValidateSignature exposes the type but not the pattern. Yes, this change isn't doing that. :)

Or would it make sense to just not allow it in any impl outside Core, or something?

We haven't generally been restricting builtin functions to Core, so I'm hesitant to start that.

toolchain/sem_ir/builtin_function_kind.cpp

toolchain/sem_ir/facet_type_info.cpp

toolchain/sem_ir/facet_type_info.h

toolchain/sem_ir/inst_namer.cpp

toolchain/sem_ir/stringify.cpp

toolchain/check/eval.cpp

toolchain/check/impl_lookup.cpp

jonmeow · 2025-09-10T21:10:00Z

@danakj Several of your comments seem to be asking for a different implementation.

At present, this can be used with where:

fn F(T:! type where .Self impls CanAggregateDestroy()) {}

That's intentional -- while I haven't figured out precisely what will work with destruction, I think we want to ability to combine special requirements with where, and potentially with other constraints. For example, we may want something like a TriviallyDestructible facet defined this way. If we disallow combining special requirements, I think we would end up with an indirection in order to be able to combine. For example:

fn TriviallyDestructibleRequirement() -> type = "type.trivially_destructible";
interface TriviallyDestructibleInterface {}

impl forall [T:! TriviallyDestructibleRequirement()] T as  TriviallyDestructibleInterface {}

...

impl forall [T:! OtherRequirements & TriviallyDestructibleInterface] T as MyInterface { ... }

In the above, creating an extra interface works, but it also creates an additional indirection.

Also,disallowing OtherRequirements & TriviallyDestructibleRequirement would be additional work in diagnostics. And as best as I can tell, the code in impl lookup wouldn't change much -- if the special requirement is the only requirement, I wasn't expecting there to be much further work to execute.

Given all your comments to this effect, should we try using one of the open discussion slots to discuss the approach here?

danakj · 2025-09-10T21:20:06Z

@danakj Several of your comments seem to be asking for a different implementation.

At present, this can be used with where:
fn F(T:! type where .Self impls CanAggregateDestroy()) {}

This is because the RHS of impls is a facet type, so this is a form of combining one facet type into another, sort of like & but it puts it in the "impls" list instead of "extends" (so there's no name lookup into it).

carbon-lang/toolchain/check/eval.cpp

Lines 2227 to 2228 in 1c6e859

    
           auto facet_type = eval_context.insts().GetAs<SemIR::FacetType>( 
        
               RequireConstantValue(eval_context, impls->rhs_id, &phase));

So that's not to say CanAggregateDestroy() is itself a where constraint, it's making a FacetType, and that FacetType is being merged into T's FacetType.

carbon-lang/toolchain/check/eval.cpp

Lines 2234 to 2241 in 1c6e859

    
           llvm::append_range(info.self_impls_constraints, 
        
                              more_info.extend_constraints); 
        
           llvm::append_range(info.self_impls_constraints, 
        
                              more_info.self_impls_constraints); 
        
           // Other requirements are copied in. 
        
           llvm::append_range(info.rewrite_constraints, 
        
                              more_info.rewrite_constraints); 
        
           info.other_requirements |= more_info.other_requirements;

That's intentional -- while I haven't figured out precisely what will work with destruction, I think we want to ability to combine special requirements with where, and potentially with other constraints. For example, we may want something like a TriviallyDestructible facet defined this way. If we disallow combining special requirements, I think we would end up with an indirection in order to be able to combine. For example:
fn TriviallyDestructibleRequirement() -> type = "type.trivially_destructible";
interface TriviallyDestructibleInterface {}

impl forall [T:! TriviallyDestructibleRequirement()] T as  TriviallyDestructibleInterface {}

...

impl forall [T:! OtherRequirements & TriviallyDestructibleInterface] T as MyInterface { ... }
In the above, creating an extra interface works, but it also creates an additional indirection.

Also,disallowing OtherRequirements & TriviallyDestructibleRequirement would be additional work in diagnostics. And as best as I can tell, the code in impl lookup wouldn't change much -- if the special requirement is the only requirement, I wasn't expecting there to be much further work to execute.

Given all your comments to this effect, should we try using one of the open discussion slots to discuss the approach here?

That is a point about combining facet types. Given that, I guess what I would like is a bit of a change but less dramatic:

Don't treat special requirements as where in textifying of facet types. Treat them more like an interface in self_impls_constraints.
Keep other_requirements separate, it's not providing an interface it's talking about constraints we couldn't capture yet.

On the topic though of "like an interface", do you expect T:! SpecialThing() and T:! type where .Self impls SpecialThing() to be different? If SpecialThing has associated constants/methods do you want them to ever participate in name lookup? If we do, then I think we need to track if the special requirement is, in spirit, part of extend_constraints (yes name lookup) or self_impls_constraints (no name lookup).

jonmeow · 2025-09-10T21:40:03Z

So that's not to say CanAggregateDestroy() is itself a where constraint, it's making a FacetType, and that FacetType is being merged into T's FacetType.

To explain, I went in this specific direction because impls requires a FacetType on the RHS. So it's a FacetType because that's what's expected in where handling, and it works in other spots because FacetType is used in other places too. It hadn't seemed worth disambiguating, but I can figure out more on that front if you want.

That is a point about combining facet types. Given that, I guess what I would like is a bit of a change but less dramatic:

Don't treat special requirements as where in textifying of facet types. Treat them more like an interface in self_impls_constraints.

Fixing this; it's not intentional, I'd just missed the extra work for self_impls_constraints to show .Self.

Keep other_requirements separate, it's not providing an interface it's talking about constraints we couldn't capture yet.

Will do.

On the topic though of "like an interface", do you expect T:! SpecialThing() and T:! type where .Self impls SpecialThing() to be different? If SpecialThing has associated constants/methods do you want them to ever participate in name lookup? If we do, then I think we need to track if the special requirement is, in spirit, part of extend_constraints (yes name lookup) or self_impls_constraints (no name lookup).

Ideally, I'd hope that these special requirements don't need to have anything. So far, we're only talking about using them just to filter types in order to determine whether there should be an impl, because that filtering is complex. If we need an interface, I'd hope that it behaves more like SpecialThing() is desugaring to multiple interfaces, one of which is defined in the prelude. It's hard to say though, without a use-case for it.

This is a bit of an experiment to see if there's a reasonable way to write a shared enum type, rather than writing per-case wrappers for things like `HasTypeQualifiers` or the printing. I think it's a bit borderline complexity right now, but I'm not sure I can reduce it much further. This changes from things like `Internal::EnumClassName##RawEnum` to `Internal::EnumClassName##Data::RawEnum` so that the enum entries can have back references to bit shifts without needing to know the containing type name. Because I'm trying to reduce duplication between mask and non-mask enums, I did this to non-mask enums too. This was motivated by #6035 adding another enum mask (which will grow more entries, and is intended to switch if this is accepted), but I'm not using that PR as a base here because I didn't want the merge dependency.

jonmeow · 2025-09-12T20:51:02Z

(note on comment times, I just now realized I hadn't published draft comments)

jonmeow · 2025-09-12T21:17:31Z

I was looking at this for EnumMaskBase changes -- WDYT of BuiltinConstraintMask instead of FacetTypeInfo::SpecialRequirementMask? I'm trying to deal with it being inconvenient to declare the mask types inside of a class (FacetTypeInfo). An alias here isn't great. I was thinking maybe the new name would be a little better for what it does anyways -- does it work for you?

toolchain/check/impl_lookup.cpp

danakj · 2025-09-12T21:11:38Z

toolchain/check/testdata/builtins/type/aggregate_destroy.carbon

+  //@dump-sem-ir-end
+}
+
+// --- todo_fail_wrong_type.carbon


isn't it the exact same signature as the Op in the test above?

toolchain/sem_ir/facet_type_info.cpp

danakj · 2025-09-12T21:15:37Z

toolchain/sem_ir/facet_type_info.h

+        rewrite_constraints.empty() && special_requirement_mask.empty() &&
+        !other_requirements) {


This means you can't write impl C as SpecialRequirementThing() which seems good. It's a little bit obfuscated though. Maybe worth a comment?

Added to BuiltinConstraintMask, along with notes about the expected API; does that seem good to you?

This did make me notice that impl C as (I & CanAggregateDestroy()) is valid, which does that seem right to you?

(either way I've added a test to show the behavior, fail_impl.carbon and impl_with_interface.carbon here)

Added to BuiltinConstraintMask, along with notes about the expected API; does that seem good to you?

This did make me notice that impl C as (I & CanAggregateDestroy()) is valid, which does that seem right to you?

Uh.. well, Idk. CanAggregeateDestroy provides the equivalent of a FacetType type where .Self impls <stuff>. We also allow this:

interface Z {} interface Y {} class C {} impl C as Y where .Self impls Z {}

and

interface Z {} interface Y {} class C {} impl C as Y & (type where .Self impls Z) {}

So I guess yes (though I am not sure that's what the design says...). It should just stay consistent with the .Self impls case if that changes.

But I was wrong about this function being the thing that limits the right side of impl as. It looks like that is the job of IdentifiedFacetType::is_valid_impl_as_target() instead.

Which raises the point that if you go from FacetTypeInfo to an IdentifiedFacetType at the moment, you lose the builtins. Maybe that's fine? They don't need to be identified and they can be looked for on the FacetTypeInfo.. but maybe also worth a comment at the least.

I think the IdentifiedFacetType behavior is fine for the moment, but adding a TODO that BuiltinConstraintMask should probably be included there. My guess is we'll want that eventually.

toolchain/sem_ir/inst_fingerprinter.cpp

toolchain/sem_ir/stringify.cpp

danakj · 2025-09-12T21:19:03Z

I was looking at this for EnumMaskBase changes -- WDYT of BuiltinConstraintMask instead of FacetTypeInfo::SpecialRequirementMask? I'm trying to deal with it being inconvenient to declare the mask types inside of a class (FacetTypeInfo). An alias here isn't great. I was thinking maybe the new name would be a little better for what it does anyways -- does it work for you?

Yes works for me

jonmeow

Catching up with comments. :)

toolchain/sem_ir/inst_fingerprinter.cpp

toolchain/sem_ir/stringify.cpp

toolchain/sem_ir/facet_type_info.cpp

jonmeow · 2025-09-12T21:27:09Z

toolchain/check/testdata/builtins/type/aggregate_destroy.carbon

+  //@dump-sem-ir-end
+}
+
+// --- todo_fail_wrong_type.carbon


Above, Self is () or {}. In this test, Self is class C. Can't that be an error in the declaration, since it's not a blanket impl?

toolchain/check/impl_lookup.cpp

jonmeow · 2025-09-12T21:40:44Z

toolchain/sem_ir/facet_type_info.h

+        rewrite_constraints.empty() && special_requirement_mask.empty() &&
+        !other_requirements) {


Added to BuiltinConstraintMask, along with notes about the expected API; does that seem good to you?

This did make me notice that impl C as (I & CanAggregateDestroy()) is valid, which does that seem right to you?

(either way I've added a test to show the behavior, fail_impl.carbon and impl_with_interface.carbon here)

danakj

LGTM

danakj · 2025-09-15T14:37:38Z

toolchain/sem_ir/facet_type_info.h

+        rewrite_constraints.empty() && special_requirement_mask.empty() &&
+        !other_requirements) {


Added to BuiltinConstraintMask, along with notes about the expected API; does that seem good to you?

This did make me notice that impl C as (I & CanAggregateDestroy()) is valid, which does that seem right to you?

Uh.. well, Idk. CanAggregeateDestroy provides the equivalent of a FacetType type where .Self impls <stuff>. We also allow this:

interface Z {} interface Y {} class C {} impl C as Y where .Self impls Z {}

and

interface Z {} interface Y {} class C {} impl C as Y & (type where .Self impls Z) {}

So I guess yes (though I am not sure that's what the design says...). It should just stay consistent with the .Self impls case if that changes.

But I was wrong about this function being the thing that limits the right side of impl as. It looks like that is the job of IdentifiedFacetType::is_valid_impl_as_target() instead.

Which raises the point that if you go from FacetTypeInfo to an IdentifiedFacetType at the moment, you lose the builtins. Maybe that's fine? They don't need to be identified and they can be looked for on the FacetTypeInfo.. but maybe also worth a comment at the least.

toolchain/sem_ir/facet_type_info.h

danakj · 2025-09-15T14:48:52Z

toolchain/check/testdata/builtins/type/aggregate_destroy.carbon

+  //@dump-sem-ir-end
+}
+
+// --- todo_fail_wrong_type.carbon


Oh I see, because C is not AggregateDestroy? Interesting, the actual interface is a builtin one (CanAggregateDestroy()). It's a bit of an extra step to also restrict which impls are allows to use the builtin function. Is the plan to require it to be in an impl which is a blanket impl and implementing the built-in interface (though this test is not)? Or would it make sense to just not allow it in any impl outside Core, or something?

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

Add builtin functions for destroy, with special requirements in facet…

d25ea05

… types

github-actions bot added the toolchain label Sep 9, 2025

github-actions bot requested a review from josh11b September 9, 2025 22:08

jonmeow force-pushed the special-reqs branch from 69eeff4 to d25ea05 Compare September 9, 2025 22:14

josh11b requested review from zygoloid and danakj and removed request for josh11b September 9, 2025 23:52

danakj reviewed Sep 10, 2025

View reviewed changes

zygoloid mentioned this pull request Sep 10, 2025

Add Copy interface and use it for making copies. #6034

Merged

review

789e58f

jonmeow mentioned this pull request Sep 11, 2025

Add an EnumMaskBase type #6053

Merged

jonmeow added 2 commits September 12, 2025 13:55

merge

4155b29

mask

2a02f10

danakj reviewed Sep 12, 2025

View reviewed changes

jonmeow added 2 commits September 12, 2025 14:27

fixing

c553a4e

updates,review

8999409

jonmeow commented Sep 12, 2025

View reviewed changes

autoupdate

6c78849

danakj approved these changes Sep 15, 2025

View reviewed changes

jonmeow and others added 4 commits September 15, 2025 08:56

Update toolchain/sem_ir/facet_type_info.h

d220052

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

Update toolchain/sem_ir/facet_type_info.h

4a02fdf

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

precommit

108adf6

todo

02842f4

merge

a3819d3

jonmeow enabled auto-merge September 15, 2025 16:35

jonmeow added this pull request to the merge queue Sep 15, 2025

Merged via the queue into carbon-language:trunk with commit 5e3bb52 Sep 15, 2025
8 checks passed

jonmeow deleted the special-reqs branch September 15, 2025 17:40

		rewrite_constraints.empty() && special_requirement_mask.empty() &&
		!other_requirements) {

Add builtin functions for destroy, with special requirements in facet types #6035

Add builtin functions for destroy, with special requirements in facet types #6035

Uh oh!

Conversation

jonmeow commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jonmeow Sep 15, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonmeow commented Sep 10, 2025

Uh oh!

danakj commented Sep 10, 2025

Uh oh!

jonmeow commented Sep 10, 2025

Uh oh!

jonmeow commented Sep 12, 2025

Uh oh!

jonmeow commented Sep 12, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonmeow Sep 12, 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!

Uh oh!

Uh oh!

danakj commented Sep 12, 2025

Uh oh!

jonmeow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonmeow Sep 12, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonmeow commented Sep 9, 2025 •

edited

Loading

jonmeow Sep 15, 2025 •

edited

Loading

jonmeow Sep 12, 2025 •

edited

Loading

jonmeow Sep 12, 2025 •

edited

Loading