-
Notifications
You must be signed in to change notification settings - Fork 10.6k
SILGen: fix cast of Optional<any Sendable> -> Optional<Any> #85472
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
base: main
Are you sure you want to change the base?
SILGen: fix cast of Optional<any Sendable> -> Optional<Any> #85472
Conversation
In some cases, at least with ObjC bridging, we could end up throwing `SGF.emitSubstToOrigValue` into an infinite recursion through `Transform::transform`, in the case of the optional-to-optional conversion: Optional<any Sendable> -> Optional<Any> There's special handling for such conversions in SILGen, and the problem is its old recognition of `any Sendable` being "equivalent" in terms of ABI to `Any` relied on type equality. While the types are not equal at compile time, they are at runtime. Plus, they are ABI equivalent. resolves rdar://163872193
|
@swift-ci smoke test |
| return false; | ||
| } | ||
|
|
||
| bool ExistentialLayout::isABICompatibleWith( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow. There are other situations where two existential types are ABI compatible but not the same type in the compiler, for example P & AnyObject vs P & C for an arbitrary class C. Does the compiler also enter an infinite loop in that case?
| init->finishInitialization(SGF); | ||
| return init->getManagedAddress(); | ||
| if (src.getType().isAddress()) | ||
| return src; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like these changes fix compiler_crashers/SILBuilder-emitStoreValueOperation-a20514.swift as well.
| auto ct1 = type1.getASTType(); | ||
| auto ct2 = type2.getASTType(); | ||
| if (ct1->isExistentialType() && ct2->isExistentialType() && | ||
| ct1->getExistentialLayout().isABICompatibleWith( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If dropping marker protocols is the correct fix here, I would prefer you used the existing withoutMarkerProtocols() (which then allows you to compare for canonical type equality after applying it to both sides) instead of adding a new method to ExistentialLayout that is only used in one place. withoutMarkerProtocols() is already used in a number of places in the compiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I second this, we have a tendency to add a lot of things that essentially do the same thing but with slight differences and it's sometimes really hard to use these APIs. withoutMarkerProtocols seems appropriate here, we use that in the Sema for the same purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although perhaps we should sink it down to TypeBase from ProtocolCompositionType, so that it can properly handle a single ProtocolType that's a marker protocol
In some cases, at least with ObjC bridging, we could end up throwing
SGF.emitSubstToOrigValueinto an infinite recursion throughTransform::transform, in the case of the optional-to-optional conversion:Optional -> Optional
There's special handling for such conversions in SILGen, and the problem is its old recognition of
any Sendablebeing "equivalent" in terms of ABI toAnyrelied on type equality.While the types are not equal at compile time, they are at runtime. Plus, they are ABI equivalent.
resolves rdar://163872193