Skip to content

Generalize the Mangling of Constrained Existential Types #59763

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

Merged
merged 1 commit into from
Jul 1, 2022

Conversation

CodaFi
Copy link
Contributor

@CodaFi CodaFi commented Jun 28, 2022

Upgrade the old mangling from a list of argument types to a
list of requiremnets. For now, only same-type requirements
may actually be mangled since those are all that are available
to the surface language.

Reconstruction of existential types now consists of demangling (a list of)
base protocol(s), decoding the constraints, and converting the same-type
constraints back into a list of arguments. Which is simple to do at the moment
because the only kinds of same-type constraints we have are rooted at
dependent types of Self.

rdar://96088707

@CodaFi CodaFi requested a review from rjmccall June 28, 2022 21:04
@CodaFi CodaFi force-pushed the mangolia branch 2 times, most recently from ea60bd9 to 620e4e5 Compare June 29, 2022 03:53
@CodaFi
Copy link
Contributor Author

CodaFi commented Jun 29, 2022

@swift-ci smoke test

appendType(EMT->getInstanceType(), sig, forDecl);
if (EMT->getInstanceType()->isExistentialType() &&
EMT->hasParameterizedExistential())
appendConstrainedExistential(EMT->getInstanceType(), sig, forDecl);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metatypes can be nested

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EMT->getInstanceType()->isExistentialType() ensures that we don't descend into them. ExistentialMetatype is only considered by isAnyExistentialType().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so if we don't need to descend into nested metatypes here, why do we need to call out direct existential metatypes at all instead of just letting the recursive visitation in appendType handle it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like, my patch to symbolically reference shapes does need to do something at this level because the existential metatype is part of the shape, but I think the normal mangling path doesn't have to. You'll just get down to the ExistentialType and do what you need to do at that level.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to call out direct existential metatypes at all instead of just letting the recursive visitation in appendType handle it?

The AST for any P<T>.Type currently looks like (ExistentialMetatypeType (ParameterizedProtocolType ...)) which is unfortunate...

$s4test3FooVAAyyAA1P_pyxqd__XPlF ---> test.Foo.test<A>(test.P<A, A1>) -> ()
$s4test3FooVAAyyAA1P_pyxXPF ---> test.Foo.test(test.P<A>) -> ()
$s4test3fooyyAA1P_px1TRtz_XPlF ---> test.foo<A>(any test.P<A.T == A>) -> ()
$s4test3fooyyAA1P_pSS1TAaCPRtz_Si1UAERtzXPF ---> test.foo(any test.P<A.test.P.T == Swift.String, A.test.P.U == Swift.Int>) -> ()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof, this demangling is super confusing. I guess we're in a generic context that says τ_0_0 is such-and-such type? Maybe we can push a different context when the LHS of these?

This ties in with my concern about using requirements here — it's really a false economy, because τ_0_0 means different things in different positions in the requirement. And you still don't have the ability to express things like P<.Slice == .self>, because you can't write τ_0_0 on the RHS without it being ambiguous in case you're actually in a generic context.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to stick with mangling requirements, you could just build them in terms of some new "protocol self" base type that's guaranteed to mangle differently from other types. Or you could use Requirement internally in the compiler when building + uniquing things and then do a special-case mangling at the last minute.

@xedin xedin changed the title Generalize the Manging of Constrained Existential Types Generalize the Mangling of Constrained Existential Types Jun 30, 2022
@CodaFi CodaFi force-pushed the mangolia branch 3 times, most recently from 22dd84b to 932ec51 Compare June 30, 2022 21:06
Upgrade the old mangling from a list of argument types to a
list of requiremnets. For now, only same-type requirements
may actually be mangled since those are all that are available
to the surface language.

Reconstruction of existential types now consists of demangling (a list of)
base protocol(s), decoding the constraints, and converting the same-type
constraints back into a list of arguments.

rdar://96088707
@CodaFi
Copy link
Contributor Author

CodaFi commented Jun 30, 2022

@swift-ci smoke test

@rjmccall
Copy link
Contributor

rjmccall commented Jul 1, 2022

Robert, you should not be merging patches like this with just smoke tests.

ahoppen added a commit to ahoppen/swift that referenced this pull request Aug 25, 2022
…s for cached type relations

swiftlang#59763 changed `appendType` to no longer be able to mangle `ParameterizedProtocolType`. Instead, we need to call into `appendConstrainedExistential` directly.

This manifested as a SourceKit crash every time you invoke code completion when you import a module that exports a type alias that resolves to a protocol with a primary associated type.

rdar://98623438
ahoppen added a commit to ahoppen/swift that referenced this pull request Aug 25, 2022
…s for cached type relations

swiftlang#59763 changed `appendType` to no longer be able to mangle `ParameterizedProtocolType`. Instead, we need to call into `appendConstrainedExistential` directly.

This manifested as a SourceKit crash every time you invoke code completion when you import a module that exports a type alias that resolves to a protocol with a primary associated type.

rdar://98623438
ahoppen added a commit to ahoppen/swift that referenced this pull request Aug 25, 2022
…s for cached type relations

swiftlang#59763 changed `appendType` to no longer be able to mangle `ParameterizedProtocolType`. Instead, we need to call into `appendConstrainedExistential` directly.

This manifested as a SourceKit crash every time you invoke code completion when you import a module that exports a type alias that resolves to a protocol with a primary associated type.

rdar://98623438
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.

2 participants