Skip to content
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

Add the notion of @_implementationOnly overrides #25447

Merged
merged 2 commits into from Jun 18, 2019

Conversation

@jrose-apple
Copy link
Contributor

jrose-apple commented Jun 14, 2019

When an @_implementationOnly import includes Objective-C categories for existing types, it's useful to be able to override the members provided in those categories without exposing them to clients of the framework being built. Allow this as long as the overriding declaration is marked as @_implementationOnly itself, with an additional check that the type of the declaration does not change. (Normally overrides are allowed to change in covariant ways.)

This is definitely not sound when not using -enable-library-evolution, and when it is using -enable-library-evolution it's probably relying on the fact that overrides of ObjC things are always dynamic and that overridable Swift members can never be added in extensions. But the feature is still underscored and there's a definite need for it.

Part of rdar://50827914

@jrose-apple

This comment has been minimized.

Copy link
Contributor Author

jrose-apple commented Jun 14, 2019

@swift-ci Please test

@jrose-apple

This comment has been minimized.

Copy link
Contributor Author

jrose-apple commented Jun 14, 2019

Follow-up work: when using @_implementationOnly, skip checking the type of a declaration too.

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Jun 14, 2019

Build failed
Swift Test Linux Platform
Git Sha - 3e1428fa8ad46d25a482d2dfcf2d1fc1b77c0aa0

if (!derivedInterfaceTy->isEqual(overrideInterfaceTy)) {
TC.diagnose(VD, diag::implementation_only_override_changed_type,
overrideInterfaceTy);
TC.diagnose(overridden, diag::overridden_here);

This comment has been minimized.

Copy link
@slavapestov

slavapestov Jun 14, 2019

Member

We tend to prefer calling diagnose() on the decl instead, I’m service of eventually removing unnecessary dependencies on the TypeChecker instance

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Jun 14, 2019

Author Contributor

Oh, I forgot that Decls have a direct diagnose now. Will update.

@jrose-apple jrose-apple force-pushed the jrose-apple:override-the-train branch 2 times, most recently Jun 14, 2019
@jrose-apple

This comment has been minimized.

Copy link
Contributor Author

jrose-apple commented Jun 14, 2019

@swift-ci Please test Linux

@jrose-apple

This comment has been minimized.

Copy link
Contributor Author

jrose-apple commented Jun 14, 2019

@swift-ci Please smoke test macOS

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Jun 14, 2019

Build failed
Swift Test Linux Platform
Git Sha - 3e1428fa8ad46d25a482d2dfcf2d1fc1b77c0aa0

@jrose-apple

This comment has been minimized.

Copy link
Contributor Author

jrose-apple commented Jun 14, 2019

@swift-ci Please test Linux

@jrose-apple

This comment has been minimized.

Copy link
Contributor Author

jrose-apple commented Jun 14, 2019

Holding off on merging until I've played with this on one of the Apple frameworks using the feature, to make sure it's behaving as expected.

@jrose-apple jrose-apple force-pushed the jrose-apple:override-the-train branch Jun 17, 2019
@jrose-apple

This comment has been minimized.

Copy link
Contributor Author

jrose-apple commented Jun 17, 2019

@swift-ci Please test

@jrose-apple jrose-apple requested a review from slavapestov Jun 17, 2019
overrideInterfaceTy =
FunctionType::get(overrideInterfaceFuncTy->getParams(),
overrideInterfaceFuncTy->getResult());
}

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Jun 17, 2019

Author Contributor

I am not happy with this whole block. Before, this section of the function was four lines that were a bit verbose but basically said what they did. Now there's a bunch of extra type munging for no obvious reason.

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Jun 17, 2019

Build failed
Swift Test Linux Platform
Git Sha - 046d6437fe3791629a4041b624be0accd3e707cc

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Jun 18, 2019

Build failed
Swift Test OS X Platform
Git Sha - 3e1428fa8ad46d25a482d2dfcf2d1fc1b77c0aa0

jrose-apple added 2 commits Jun 14, 2019
When an @_implementationOnly import includes Objective-C categories
for existing types, it's useful to be able to override the members
provided in those categories without exposing them to clients of the
framework being built. Allow this as long as the overriding
declaration is marked as @_implementationOnly itself, with an
additional check that the type of the declaration does not change.
(Normally overrides are allowed to change in covariant ways.)

Part of rdar://50827914
And improve the comment around the FIXME for this, which is tricky to
implement.
@jrose-apple jrose-apple force-pushed the jrose-apple:override-the-train branch to 44227fe Jun 18, 2019
@jrose-apple

This comment has been minimized.

Copy link
Contributor Author

jrose-apple commented Jun 18, 2019

@swift-ci Please test

1 similar comment
@jrose-apple

This comment has been minimized.

Copy link
Contributor Author

jrose-apple commented Jun 18, 2019

@swift-ci Please test

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Jun 18, 2019

Build failed
Swift Test OS X Platform
Git Sha - 39971ef551f88936986b1da25d975e84db266f3b

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Jun 18, 2019

Build failed
Swift Test Linux Platform
Git Sha - 39971ef551f88936986b1da25d975e84db266f3b

@jrose-apple jrose-apple merged commit 585ebbd into apple:master Jun 18, 2019
4 checks passed
4 checks passed
Swift Test Linux Platform No test results found.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform No test results found.
Details
Swift Test OS X Platform (smoke test)
Details
@jrose-apple jrose-apple deleted the jrose-apple:override-the-train branch Jun 18, 2019
jrose-apple added a commit to jrose-apple/swift that referenced this pull request Jun 18, 2019
Add the notion of @_implementationOnly overrides

(cherry picked from commit 585ebbd)
jrose-apple added a commit that referenced this pull request Jun 19, 2019
Add the notion of @_implementationOnly overrides

(cherry picked from commit 585ebbd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.