Skip to content

[Serialization] Store whether an override depends on its base for ABI #27784

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

Conversation

jrose-apple
Copy link
Contributor

In some circumstances, a Swift declaration in module A will depend on another declaration (usually from Objective-C) that can't be loaded, for whatever reason. If the Swift declaration is overriding the missing declaration, this can present a problem, because the way methods are dispatched in Swift can depend on knowing the original class was that introduced the method. However, if the compiler can prove that the override can still be safely invoked/used in all cases, it doesn't need to worry about the overridden declaration being missing.

This is especially relevant for property accessors, because there's currently no logic to recover from a property being successfully deserialized and then finding out that an accessor couldn't be.

The decision of whether or not an override can be safely invoked without knowledge of the base method is something to be cautious about—a mistaken analysis would effectively be a miscompile. So up until now, this was limited to one case: when a method is known to be @objc dynamic, i.e. always dispatched through objc_msgSend. (Even this may become questionable if we have first-class method references, like we do for key paths.) This worked particularly well because the compiler infers dynamic for any overload of an imported Objective-C method or accessor, in case it imports differently in a different -swift-version and a client ends up subclassing it.

However...that inference does not apply if the class is final, because then there are no subclasses to worry about.

This commit changes the test to be more careful: if the missing declaration was @objc dynamic, we know that it can't affect ABI, because either the override is properly @objc dynamic as well, or the override has introduced its own calling ABI (in practice, a direct call for final methods) that doesn't depend on the superclass. Again, this isn't 100% correct in the presence of first-class methods, but it does fix the issue in practice where a property accessor in a parent class goes missing. And since Objective-C allows adding property setters separately from the original property declaration, that's something that can happen even under normal circumstances. Sadly.

This approach could probably be extended to constructors as well. I'm a little more cautious about throwing vars and subscripts into the mix because of the presence of key paths, which do allow identity-based comparison of overrides and bases.

rdar://problem/56388950

@jrose-apple
Copy link
Contributor Author

@swift-ci Please test

@jrose-apple
Copy link
Contributor Author

@swift-ci Please test source compatibility

@jrose-apple jrose-apple requested a review from xymus October 18, 2019 23:46
@varungandhi-apple
Copy link
Contributor

Don't have time to review this right away but there's a typo in the commit message:

on knowing the original class was that introduced the method.


Again, this isn't 100% correct in the presence of first-class methods, but it does fix the issue in practice where a property accessor in a parent class goes missing

My understanding around keypaths is a bit fuzzy, but doesn't the fact that you can use keypaths to access properties (correct?), and that properties can be overriden mean that you kinda' have first-class methods for some limited types (T) -> Void and () -> T?

And since Objective-C allows adding property setters separately from the original property declaration

screaming

Copy link
Contributor

@jckarter jckarter left a comment

Choose a reason for hiding this comment

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

Makes sense to me.

@jrose-apple
Copy link
Contributor Author

My understanding around keypaths is a bit fuzzy, but doesn't the fact that you can use keypaths to access properties (correct?), and that properties can be overriden mean that you kinda' have first-class methods for some limited types (T) -> Void and () -> T?

That's sorta the point: I'm nervous about applying the same logic to VarDecls because they can be referenced in ways that require an additional symbol at runtime besides their accessors.

In some circumstances, a Swift declaration in module A will depend on
another declaration (usually from Objective-C) that can't be loaded,
for whatever reason. If the Swift declaration is *overriding* the
missing declaration, this can present a problem, because the way
methods are dispatched in Swift can depend on knowing the original
class that introduced the method. However, if the compiler can prove
that the override can still be safely invoked/used in all cases, it
doesn't need to worry about the overridden declaration being missing.

This is especially relevant for property accessors, because there's
currently no logic to recover from a property being successfully
deserialized and then finding out that an accessor couldn't be.

The decision of whether or not an override can be safely invoked
without knowledge of the base method is something to be cautious
about---a mistaken analysis would effectively be a miscompile. So up
until now, this was limited to one case: when a method is known to be
`@objc dynamic`, i.e. always dispatched through objc_msgSend. (Even
this may become questionable if we have first-class method references,
like we do for key paths.) This worked particularly well because the
compiler infers 'dynamic' for any overload of an imported Objective-C
method or accessor, in case it imports differently in a different
-swift-version and a client ends up subclassing it.

However...that inference does not apply if the class is final, because
then there are no subclasses to worry about.

This commit changes the test to be more careful: if the /missing/
declaration was `@objc dynamic`, we know that it can't affect ABI,
because either the override is properly `@objc dynamic` as well, or
the override has introduced its own calling ABI (in practice, a direct
call for final methods) that doesn't depend on the superclass. Again,
this isn't 100% correct in the presence of first-class methods, but it
does fix the issue in practice where a property accessor in a parent
class goes missing. And since Objective-C allows adding property
setters separately from the original property declaration, that's
something that can happen even under normal circumstances. Sadly.

This approach could probably be extended to constructors as well. I'm
a little more cautious about throwing vars and subscripts into the mix
because of the presence of key paths, which do allow identity-based
comparison of overrides and bases.

rdar://problem/56388950
@jrose-apple jrose-apple force-pushed the a-series-of-unfortunate-xrefs branch from a8e3c55 to 138e558 Compare October 21, 2019 18:42
@jrose-apple
Copy link
Contributor Author

Fixed commit message typo.

@swift-ci Please smoke test

@jrose-apple
Copy link
Contributor Author

@swift-ci Please smoke test

@jrose-apple
Copy link
Contributor Author

@swift-ci Please smoke test Linux

@jckarter
Copy link
Contributor

That's sorta the point: I'm nervous about applying the same logic to VarDecls because they can be referenced in ways that require an additional symbol at runtime besides their accessors.

Well, the goal of the key path property descriptor is to abstract the details of the property implementation from other modules. The only thing that should affect the descriptor's ABI is the type of the property.

@jrose-apple
Copy link
Contributor Author

Well, the goal of the key path property descriptor is to abstract the details of the property implementation from other modules. The only thing that should affect the descriptor's ABI is the type of the property.

Ah, do \Sub.foo and \Base.foo have different symbols? That would make me feel better about a future expansion to VarDecl as well.

@jrose-apple jrose-apple merged commit a52fac4 into swiftlang:master Oct 21, 2019
@jrose-apple jrose-apple deleted the a-series-of-unfortunate-xrefs branch October 21, 2019 22:53
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.

3 participants