[Variadic Generics] Always use PackType as the substitution for type parameter packs.
#64166
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This simplifies the representation and allows clients to handle fewer cases. It also removes an ambiguity in the representation which could lead us to have two canonical types for the same type. This is definitely not working yet, but I'm not making progress on it quickly enough to unblock what we need to unblock; it'll have to be fixed in parallel.
…d pack element environments. This allows the constraint system to ensure that for a given pack expansion locator, the given shape class is always the same when requesting the element environment. If the shape class differs, it means there's a same-shape requirement failure, which will be diagnosed via the ShapeOf constraint simplification.
The rewrite rules are not quite right yet for same-element requirements, so let's ban them for now.
…etonPackExpansion, and update variadic generics SILGen tests for the representation change.
hborla
commented
Mar 7, 2023
| if (!gp->isParameterPack()) | ||
| return archetype; | ||
|
|
||
| return PackType::getSingletonPackExpansion(archetype); |
There was a problem hiding this comment.
@slavapestov I tried changing this to:
return SubstitutionMap::get(
nominal->getGenericSignatureOfContext(),
genericEnv->getForwardingSubstitutionMap());but it still crashed when building the standard library. I didn't spend much time figuring out why because I wanted to finish the PackType representation change, but I can come back to it.
|
@swift-ci please smoke test |
|
@swift-ci please smoke test |
slavapestov
reviewed
Mar 7, 2023
| @@ -920,6 +920,10 @@ static Type applyGenericArguments(Type type, TypeResolution resolution, | |||
| auto arg = found->rhs; | |||
| if (auto *expansionType = arg->getAs<PackExpansionType>()) | |||
| arg = expansionType->getPatternType(); | |||
|
|
|||
| if (arg->isParameterPack()) | |||
There was a problem hiding this comment.
PackMatcher should be refactored to do this. Right now it unwraps one element packs, and here we re-wrap them
There was a problem hiding this comment.
Ah, I'll fix that, thanks!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Canonicalize the representation of type parameter pack substitutions as
PackType, instead of sometimes usingPackTypeand sometimes usingPackArchetypeType.Subsumes #63908