Expand TypeCanDestroy to handle more types #6164
Conversation
jonmeow
force-pushed
the
destroy-more
branch
2 times, most recently
from
723c9a9 to
ca6eb8d
Compare
jonmeow
force-pushed
the
destroy-more
branch
2 times, most recently
from
c45f33e to
8b66dca
Compare
core/prelude/types/optional.carbon
Outdated
| // TODO: We don't have an approved design for an `Optional` type yet, but it's | ||
| // used by the design for `Iterate`. The API here is a placeholder. | ||
| class Optional(T:! OptionalStorage) { | ||
| class Optional(T:! Destroy & OptionalStorage) { |
There was a problem hiding this comment.
I'm not necessarily opposed to this constraint, but I wonder it would be better for this to be a constraint on the Destroy impl rather than on the type -- that is, that Optional(T) should implement Destroy only if T implements Destroy rather than requiring that T impls Destroy. I don't think we're destroying a T as part of the definition of Optional (other than in its Destroy impl).
There was a problem hiding this comment.
Done. This lingered from the earlier Optional approach, and I didn't think about it while updating.
| fn G(T:! Core.Destroy) { | ||
| // We can initialize this without knowing T. | ||
| //@dump-sem-ir-begin | ||
| var arr: array(T, 0) = (); |
There was a problem hiding this comment.
I wonder if arrays with bound 0 should be a special case and not require T to be destroyable here?
There was a problem hiding this comment.
Is there a use-case for that? If not I'd suggest not special casing... it seems to not be supported in C++ (https://cpp.compiler-explorer.com/z/98q6c1e7K) so perhaps niche enough it's not needed.
A 0-sized array seems like a niche use-case. My guess would be the most common use would be something like a generic where the array size is a parameter, and often 0. But such a thing would typically still need to support non-0-sized arrays (if they're special-casing only nondestroyable types as a 0-sized array, it seems they could equally special-case some other way)
| auto obj_repr_id = | ||
| class_info.GetObjectRepr(context.sem_ir(), SemIR::SpecificId::None); |
There was a problem hiding this comment.
| auto obj_repr_id = | |
| class_info.GetObjectRepr(context.sem_ir(), SemIR::SpecificId::None); | |
| auto obj_repr_id = | |
| class_info.GetObjectRepr(context.sem_ir(), class_type.specific_id); |
I think this might allow you to undo a bunch of the test changes.
There was a problem hiding this comment.
This doesn't work right now, adding a TODO
toolchain/check/impl_lookup.cpp
Outdated
| case CARBON_KIND(SemIR::BaseDecl base_decl): { | ||
| work.push_back( | ||
| {.id = base_decl.base_type_inst_id, .allow_abstract = true}); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Is this reachable? We seem to generally only have types on the worklist, and BaseDecl isn't a type.
There was a problem hiding this comment.
Removed, must've been lingering.
| default: | ||
| CARBON_FATAL("TypeCanDestroy found unexpected inst: {0}", inst); |
There was a problem hiding this comment.
Can we arrange this so that we get a build failure if someone adds a new type for which is_type is not InstIsType::Never and misses it here?
There was a problem hiding this comment.
Is that desirable? There's a lot of InstIsType::Maybe and I'm not clear they can occur here
There was a problem hiding this comment.
(I think ImplWitnessAccess is the only Maybe I'm seeing as required to handle, at least for now)
There was a problem hiding this comment.
(and I assume it'd require refactoring into an overload dispatch to make a build failure)
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
2 participants
This is modifying the
TypeCanDestroyimplementation to more closely examine types, in particular making it more sensitive about type structure. Noteabstractcannot be destroyed per #6124, andpartialis a question per #6161 but this makes it destructible for consistency with existing tests.Note this leaves TODOs to dig into types further. This PR led to substantial work solving issues in generics handling, so trying to get it in partly to prevent regressions.