[Swiftify] don't add macro when required type is unavailable #86084
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Author
|
@swift-ci please smoke test |
1 similar comment
Member
Author
|
@swift-ci please smoke test |
hnrklssn
force-pushed
the
opaque-non-opaque
branch
from
1e2c9de to
c2f2306
Compare
A Swift module only imports a type definition once. This means that some clang modules may have function signatures imported with `UnsafePointer<qux>` even if `qux` is only forward declared in that module (which would normally be imported as `OpaquePointer`), because some Swift file in the module imported the clang module with the definition, so ClangImporter sees the definition. The macro expansion only imports the imports of the clang module it belongs to however, so namelookup for `qux` fails. This patch detects when this will result in an error and avoids attaching the macro in those cases. rdar://165864358
Implicit decls don't have an owning module, since multiple modules could instantiate them on demand. Since no implicit functions currently have any need for safe wrappers we can simply detect this and early exit. For templated functions, we need to fetch the owning module from the instantiation.
hnrklssn
force-pushed
the
opaque-non-opaque
branch
from
c2f2306 to
cd5d8e1
Compare
Member
Author
|
@swift-ci please smoke test |
This fixes an assert when signatures contain
`struct forward_declared_t **`, and makes sure that
`struct foo { struct forward_declared_t *bar; }` does *not* block
swiftification, since `@_SwiftifyImport` does not need to access the
struct field.
Member
Author
|
@swift-ci please smoke test |
Since decls with no module are imported to __ObjC this is not an issue, because __ObjC is always implicitly imported. Added test case that would previously assert.
Member
Author
|
@swift-ci please smoke test |
hnrklssn
requested review from
Xazax-hun,
beccadax,
egorzhdan,
ian-twilightcoder,
j-hui and
susmonteiro
as code owners
j-hui
approved these changes
Dec 19, 2025
Xazax-hun
approved these changes
Dec 19, 2025
Contributor
Xazax-hun
left a comment
Xazax-hun
left a comment
There was a problem hiding this comment.
Overall looks good to me, one question inline.
| Action walkToTypePre(Type ty) override { | ||
| DLOG("Walking type:\n"); | ||
| LLVM_DEBUG(DUMP(ty)); | ||
| if (Type PointeeTy = ConcretePointeeType(ty)) { |
Contributor
There was a problem hiding this comment.
Could we ever run into trouble with UnsafeBufferPointer and co?
Member
Author
There was a problem hiding this comment.
As in, could we ever encounter an UnsafeBufferPointer in the imported signature? Not that I know of
egorzhdan
reviewed
Dec 19, 2025
Contributor
egorzhdan
left a comment
egorzhdan
left a comment
There was a problem hiding this comment.
Haven't looked into the specifics of the implementation, but the approach looks good, thanks!
Member
Author
|
@swift-ci please smoke test windows platform |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A Swift module only imports a type definition once. This means that some clang modules may have function signatures imported with
UnsafePointer<qux>even ifquxis only forward declared in that module (which would normally be imported asOpaquePointer), because some Swift file in the module imported the clang module with the definition, so ClangImporter sees the definition. The macro expansion only imports the imports of the clang module it belongs to however, so namelookup forquxfails. This patch detects when this will result in an error and avoids attaching the macro in those cases.rdar://165864358