Skip to content

Conversation

rjmccall
Copy link
Contributor

@rjmccall rjmccall commented Mar 23, 2023

The existing type substitution logic performs pack expansion during substitution by applying a series of transforms:

  • First, it naively replaces all pack archetypes with the corresponding pack substitution. This creates pack expansions where the shape type is directly a pack and the pattern type may contain packs in positions where packs are not normally allowed ( i.e. most positions). For example, to use a slightly more explicit syntax than the proposal, repeat<T> (each T) -> Bool might become repeat<Pack{Int, repeat<U> each U}> (each Pack{Int, repeat<U> each U}) -> Bool.
  • Next, it recognizes pack expansions where the shape type is a pack but the pattern type is not. For these, it expands the pattern into a pack with one element per element of the shape pack, replacing packs in the shape with the element (or element pattern, if a pack expansion) at the corresponding index. If the shape component is itself a pack expansion, the pattern pack component becomes an expansion over the same shape. Our example above would turn into repeat<Pack{Int, repeat<U> each U}> Pack{(Int) -> Bool, repeat (each U) -> Bool}.
  • Finally, transforms on tuples, function parameters, and packs recognize when they've produced a component that's a pack expansion where the shape and pattern are both packs, and they expand the components of the pack into the surrounding type. So if the repeat in our example above was a singleton tuple element, the tuple would become ((Int) -> Bool, repeat<U> (each U) -> Bool), which is the correct final type.
    This approach works, and I think it could still be made to work in the future where our representation properly supports nested expansions with PackElementType. However, it has several flaws. First, it is a pretty intricate dance, and if the inputs are wrong (e.g. you set up a substitution incorrectly), the dance can fail in inexplicable ways. Second, it relies on pack types being temporary artifacts in the representation; if the second step ever sees a pack that is not a temporary artifact of this dance, it is likely to misbehave. Third, it temporarily constructs types that would otherwise be invalid, which means we cannot easily catch bugs by asserting that the representation is well-formed in ways that only apply to the "steady state", e.g. that types in certain positions are never pack types. Fourth, it generates a fair number of "garbage" types that will be unused after the traversal, which increases memory usage. And finally, when we move to PackElementType, we will need to understand when we're substituting inside of a pack expansion anyway in order to adjust the replacement type, destroying any simplicity benefits of the representation.

Instead, we need to handle pack expansions specially in substitution and recurse separately for each substituted component of the shape. Note that we recognize two kinds of pack reference: pack references that occur in the shape of a pack expansion, which always refer to the full pack, and pack references that occur in patterns of pack expansions, which refer to the current element of that expansion. This will become more explicit when we introduce PackElementType, which represents the latter.

There were a couple places that were already using this new approach before this PR. When they needed to substitute a type (an unlowered one in the latter case), they passed functions down to the normal AST type substitution logic which projected the current component. This has a basic conceptual problem in that it cannot handle nested expansions correctly that occur entirely outside of the substituter. I ran into this in the SIL type substituter, but it could also have happened with conformance packs. The two approaches do not play nicely with each other, and expanding the AST expansion approach to SIL types would not have worked because SIL type substitution is inherently position-sensitive: we need to lower type substitutions that appear in lowered-type positions, but we cannot lower them in formal-type positions. Therefore we must change the existing AST substitution logic to start doing element-wise substitution. This requires preserving information throughout substitution about both the original pack substitution and the currently-selected element for a particular expansion.

Because the old expansion code also has to be replaced, type simplification in the constraint system needs to use similar element-wise resolution.

@rjmccall rjmccall marked this pull request as draft March 23, 2023 17:49
@rjmccall
Copy link
Contributor Author

@swift-ci Please smoke test

@rjmccall rjmccall force-pushed the in-flight-substitution branch 2 times, most recently from d1b941f to a142bbe Compare March 25, 2023 22:54
There are a lot of problems caused by our highly-abstract substitution
subsystem.  Most of them would be solved by a more semantic / holistic
understanding of the active transformation, but that's difficult to do
because we just pass around function_refs.  The first step in fixing
that is to pass around a better currency type.  For now, it can just
hold the function_refs (and the SubstOptions).

I've set it up so that the places that just apply SubstitutionMaps
are constructing the IFS in a standard way; that should make it easy
to change those places in the future.
@rjmccall rjmccall force-pushed the in-flight-substitution branch from a142bbe to 1266ca0 Compare March 25, 2023 22:54
@rjmccall
Copy link
Contributor Author

@swift-ci Please test

@rjmccall rjmccall changed the title [NFC] Thread an InFlightSubstitution type through the substitution subsystem Substitute pack expansions by repeatedly substituting the pattern type Mar 25, 2023
Substitution of a pack expansion type may now produce a pack type.
We immediately expand that pack when transforming a tuple, a function
parameter, or a pack.

I had to duplicate the component-wise transformation logic in the
simplifyType transform, which I'm not pleased about, but a little
code duplication seemed a lot better than trying to unify the code
in two very different places.

I think we're very close to being able to assert that pack expansion
shapes are either pack archetypes or pack parameters; unfortunately,
the pack matchers intentionally produce expansions of packs, and I
didn't want to add that to an already-large patch.
@rjmccall rjmccall force-pushed the in-flight-substitution branch from 1266ca0 to 251c089 Compare March 26, 2023 08:13
@rjmccall rjmccall marked this pull request as ready for review March 26, 2023 08:13
@rjmccall rjmccall requested a review from AnthonyLatsis as a code owner March 26, 2023 08:13
@rjmccall
Copy link
Contributor Author

@swift-ci Please test

@rjmccall rjmccall merged commit b73143a into swiftlang:main Mar 26, 2023
@rjmccall rjmccall deleted the in-flight-substitution branch March 26, 2023 16:30
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.

1 participant