-
Notifications
You must be signed in to change notification settings - Fork 10.6k
Introduce "-internal" variant of bridging header import flags #84410
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
Introduce "-internal" variant of bridging header import flags #84410
Conversation
"'%select{private|fileprivate|internal|package|%ERROR|%ERROR}1' " | ||
"from %2", | ||
(const Decl *, AccessLevel, const ModuleDecl*)) | ||
"from %select{%2|bridging header}3", |
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 feel like there may be more resilience diagnostics that will need to be updated, but it seems like this is a good start
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.
Perhaps! They'll come out as the weird __ObjC module if we don't customize them.
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.
A new kind of DisallowedOriginKind
may do the trick here.
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.
Ah, that could help here, yes.
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 did this in b7a8349
lib/Sema/ImportResolution.cpp
Outdated
ImportedModule(headerModule), SourceLoc(), ImportFlags::Exported); | ||
if (ctx.ClangImporterOpts.BridgingHeaderIsInternal) { | ||
import.accessLevel = AccessLevel::Internal; | ||
import.options |= ImportFlags::ImplementationOnly; |
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.
The mix of @_implementationOnly internal import
isn't exactly an expected configuration at the moment. I'm worried that checks for both could overlap and duplicate the exportability diagnostics. We could use only internal import
for library-evolution enabled modules if it's an issue.
Do we still see the warnings on implementation-only use from non-library-evolution modules for an implicit import like this?
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.
Hmm, looks like we don't need both internal
and @_implementationOnly
to get the diagnostics I've been checking. I'll remove it for now.
…ad of `-import-objc-header` This command-line option hasn't been Objective-C specific ever, really. Make the language-independent spelling the primary one to make that more obvious.
The flags "-import-bridging-header" and "-import-pch" import a bridging header, treating the contents as a public import. Introduce "internal-" variants of both flags that provide the same semantics, but are intended to treat the imported contents as if they came in through an internal import. This is just plumbing of the options for the moment.
When using an internal import for a bridging header, semantically treat the contents of the bridging header, and anything that it imports, as if they were imported internally. This is the actual semantic behavior we wanted from internally-imported bridging headers. This is the main semantic checking bit for rdar://74011750.
C++ namespaces always get imported into the bridging header's module for Reasons. Exempt them from internal-import checking, since we get checking for the namespace members that are actually used.
…tion It's very, very easy to make a mistake that will cause broken serialized modules. Until that's no longer true, at least tell folks that they are heading into uncharted waters, as we do with `@_implementationOnly` imports.
Internally-imported bridging headers must not leak outside of the Swift module. Don't serialize their contents, and make sure we can still import the module even if the bridging header has been removed.
…ernal bridging headers
84a2d2c
to
7bc02d6
Compare
@swift-ci please smoke test |
@swift-ci please smoke test macOS |
@swift-ci clean smoke test macOS |
@swift-ci clean smoke test macOS |
@swift-ci please smoke test |
@swift-ci please smoke test Windows |
@swift-ci please smoke test |
Can we add a feature flag for this in lib/Option/features.json for swift-build to pick up? (There's still a few cases where they don't use swift-driver) |
@swift-ci please smoke test Windows |
@swift-ci please smoke test |
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.
This is awesome, thank you so much for doing this!
4ebc6dd
to
87cbe5d
Compare
@swift-ci please smoke test |
Instead of making a separate |
I'd rather not, because having |
The flags "-import-bridging-header" and "-import-pch" import a bridging header, treating the contents as a
public
import. Introduce "internal-" variants of both flags that provide the same semantics, but treat the imported contents as if they came in through an@_implementationOnly internal
import instead. This helps us keep the the bridging header as an implementation detail.This is only plausibly safe with library evolution enabled, so warn about that.
Tracked by rdar://74011750.