[cxx-interop] Remove annotationOnly flag from Escapability request #85643
Conversation
susmonteiro
requested review from
Xazax-hun,
beccadax,
hnrklssn,
ian-twilightcoder and
j-hui
as code owners
susmonteiro
changed the title
|
@swift-ci please smoke test |
| type->isMemberPointerType() || type->isReferenceType()) { | ||
| // pointer and reference types are currently imported as unknown | ||
| // (importing them as non-escapable broke backward compatibility) | ||
| hasUnknown = true; |
There was a problem hiding this comment.
I'm fine with this for now, to preserve the current behavior, but assuming pointers have unknown escapability seems like it could be problematic. My understanding is that, semantically, pointers should be treated as non-escapable.
Do we have a plan for how to correctly analyze these types while maintaining backwards compat?
An idea I had was making the analysis depend on whether strict memory safety is turned on or off, essentially using/abusing it as a feature flag. But that may not be a good idea for reasons that I haven't fully thought through yet.
There was a problem hiding this comment.
Do we have a plan for how to correctly analyze these types while maintaining backwards compat?
I do not think we can maintain backward compatibility and be precise at the same time. And we do not want strict memory safety to influence the behavior of the code. Making a type suddenly non-escapable when strict memory safety is on would result in some code not compiling. Especially when one is using an API that is owned by someone else this compilation error might be impossible to resolve.
We would not be able to do this any time soon. I tried this and it breaks a lot of code, including code in the standard library. Unfortunately, however, non-escapable types are still not first class citizens in Swift. They are viral, so suddenly even some Swift types would become non-escapable, and many protocols have not been generalized to work with non-escapable types. So the very least we need to wait until non-escapable gets some improvements, and even after that the switch would be really expensive so we will need to demonstrate a lot of value to justify this. |
Xazax-hun
left a comment
Xazax-hun
left a comment
There was a problem hiding this comment.
Overall looks good. Feel free to merge once the nits are resolved.
susmonteiro
force-pushed
the
susmonteiro/remove-annotationonly-flag
branch
from
6279d81 to
146726b
Compare
|
@swift-ci please smoke test |
susmonteiro
force-pushed
the
susmonteiro/remove-annotationonly-flag
branch
from
146726b to
27e0ae8
Compare
|
@swift-ci please smoke test |
| // We only infer escapability for simple types, such as aggregates and | ||
| // RecordDecls that are not CxxRecordDecls. For more complex | ||
| // CxxRecordDecls, we rely solely on escapability annotations. |
There was a problem hiding this comment.
In C++ language mode, Clang makes all RecordDecls into CXXRecordDecls. Could this cause source compatibility issues with Obj-C interop? e.g. if someone enables C++ interop for the first time, would some types have different escapability?
There was a problem hiding this comment.
I'd expect those record decls to be aggregates in C++, so we should handle them the same way.
|
@swift-ci please smoke test macos |
|
@swift-ci please smoke test |
susmonteiro
force-pushed
the
susmonteiro/remove-annotationonly-flag
branch
from
27e0ae8 to
b06631e
Compare
|
@swift-ci please smoke test |
4 participants
The
annotationOnlyflag was originally introduced to control for which types we would infer escapability, and for which we would rely solely on annotations. However, due to backward compatibility issues, this flag is currently always set totrue. Therefore, this patch removes theannotationOnlyflag and makes the compiler infer escapability from bases and fields only for simple records, such as aggregates.