Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NFC] Rework the internal design of SIL type lowering. #64099

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rjmccall
Copy link
Member

@rjmccall rjmccall commented Mar 4, 2023

The old, CRTP-based design was really difficult to understand because of how it tried to map type-based visitation into property-based handling. That didn't really work out for producing TypeLowering objects anyway, since a lot of different type kinds want their own TypeLowering handling even if they share abstract properties. That in turn led to a lot of redundancy between the two users, and that redundancy tended to lead to bugs --- I got into this because I kept finding problems where the pack-related types had different properties on different paths.

That's bad enough, really, but there were other problems, too. Classification basically repeated the entire AbstractionPattern-based walk over the type that lowering does, and I'm not sure why --- there aren't actually any type properties that vary by abstraction pattern, and I'm not sure why there ever would be. Needing that abstraction pattern clutters up the API and implementation a lot, especially when working with lowered types. And the way that property computation was directly tied to producing a TypeLowering object meant that preserving certain kinds of top-down property was unnecessarily cumbersome and error-prone. For example, the code was passing down a bit to remember that we'd looked through an opaque archetype so that it would get persisted in the is-expansion-sensitive bit; that's trivial to handle in the one case if property computation is independent. We also weren't set up to answer any of the basic lowering questions (type properties, lowered type) without vending a TypeLowering object, which is just an unnecessary expense (and we probably ought to get rid of those objects completely someday).

(The old design was entirely my fault, just to be clear here.)

Anyway, we can instead separate these into three related actions:

  • producing a lowered type
  • computing the properties of a lowered or formal type
  • creating a TypeLowering object

Each of these operations is then relatively straightforward to implement and largely stands separate from the others. And we can make queries for lowered types and certain type properties answerable without getting a TypeLowering, although I have not changed any clients yet to stop asking for a TypeLowering unnecessarily.

I did have an initial design for type property computation which relied on being given a lowered type, but that was unnecessary, expensive, and caused problems with the infinite-type check.

I also removed the thing where type lowering would find generic signatures for interface substTypes in the abstraction pattern. This kind of idea is absolutely asking for trouble with confusion between generic signatures — the abstraction pattern is its own type, and the subst type is a sort of substitution of that which can be in terms of a different signature entirely. I suspect we've been getting away with it mostly because relatively little code actually needs to lower (or ask about the type properties of) interface types. Anyway, this required updating a few clients to use GenericContextRAII correctly, but the result is so much better and more obviously correct.

Oh, and I added move-only-ness as a type property, although I'm not sure my logic is completely correct in the presence of stuff like SILMoveOnlyWrappedType.

The old, CRTP-based design was really difficult to understand
because of how it tried to map type-based visitation into property-based
handling.  That didn't really work out for producing TypeLowering
objects anyway, since a lot of different type kinds want their own
TypeLowering handling even if they share abstract properties.
That in turn led to a lot of redundancy between the two users,
and that redundancy tended to lead to bugs --- I got into this
because I kept finding problems where the pack-related types had
different properties on different paths.

That's bad enough, really, but there were other problems, too.
Classification basically repeated the entire AbstractionPattern-based
walk over the type that lowering does, and I'm not sure why --- there
aren't actually any type properties that vary by abstraction pattern,
and I'm not sure why there ever would be.  And the way that property
computation was directly tied to producing a TypeLowering object
meant that preserving certain kinds of top-down property was
unnecessarily cumbersome, like remembering that we looked through
an opaque archetype in the is-expansion-sensitive bit.  We also
weren't set up to answer any of the basic lowering questions (type
properties, lowered type) without vending a TypeLowering object,
even though we mostly do not need TypeLowering objects in clients
(and probably ought to get rid of them completely someday).

(The old design was entirely my fault, just to be clear here.)

Anyway, we can instead separate these into three related actions:
- producing a lowered type
- computing the properties of a lowered or formal type
- creating a TypeLowering object

Each of these operations is then relatively straightforward to implement
and largely stands separate from the others.  And we can make queries
for lowered types and certain type properties answerable without getting
a TypeLowering, although I have not changed clients to stop asking for a
TypeLowering unnecessarily.

I did have an initial design for type property computation which relied
on being given a lowered type, but that was unnecessary, expensive, and
caused problems with the infinite-type check.  There's no reason we can't
answer questions about type properties with just a type; we don't even
need an abstraction pattern for this.  (We do need a generic signature,
if the type contains type parameters.)

I removed the previous thing where it would find generic signatures for
interface substTypes in the abstraction pattern.  This required updating
a few clients, but the result is *so* much better and more obviously
correct.

Oh, and I added move-only-ness as a type property, although I'm not
sure my logic is completely correct in the presence of stuff like
SILMoveOnlyWrappedType.
@rjmccall
Copy link
Member Author

rjmccall commented Mar 4, 2023

@swift-ci Please test

@slavapestov
Copy link
Member

Neat!

@rjmccall
Copy link
Member Author

rjmccall commented Mar 4, 2023

Okay, I think this is probably just doing too much. I'll come back to it in a few days and see if I can do the internal reorganization without intentionally changing anything in the API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants