Skip to content

[MacrosOnImports][Swiftify] Copy module imports from clang node's module to its Swift macro SourceFile #81859

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

hnrklssn
Copy link
Contributor

@hnrklssn hnrklssn commented May 30, 2025

Since the _SwiftifyImport peer macro expansion reuses the function signature of the original imported function, it needs to import the same modules as the module where the original clang node resides, to ensure visibility of any symbols that may appear in the signature.

rdar://151611573

@hnrklssn
Copy link
Contributor Author

hnrklssn commented Jun 4, 2025

@swift-ci please smoke test


// CHECK: import ModuleA
// CHECK-NEXT: import ModuleB
// CHECK-NOT: import
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DougGregor I don't fully get why only the module's imports that themselves are top-level modules show up in the dump. It seems to work regardless, but it seems a bit off. Do you know if this is intended?

@hnrklssn hnrklssn force-pushed the swiftify-inherit-imports branch from 7d2431e to e5c5c6e Compare June 4, 2025 04:35
@hnrklssn
Copy link
Contributor Author

hnrklssn commented Jun 4, 2025

@swift-ci please smoke test

@hnrklssn hnrklssn force-pushed the swiftify-inherit-imports branch from e5c5c6e to 229280f Compare June 5, 2025 01:17
@hnrklssn
Copy link
Contributor Author

hnrklssn commented Jun 5, 2025

@swift-ci please smoke test

@hnrklssn hnrklssn requested a review from CodaFi June 5, 2025 01:18
@hnrklssn
Copy link
Contributor Author

hnrklssn commented Jun 5, 2025

@swift-ci please smoke test windows

@hnrklssn
Copy link
Contributor Author

hnrklssn commented Jun 5, 2025

@swift-ci please smoke test

@hnrklssn hnrklssn force-pushed the swiftify-inherit-imports branch from 1b8ba84 to 7c794aa Compare June 7, 2025 21:56
@hnrklssn hnrklssn requested a review from xymus as a code owner June 7, 2025 21:56
@hnrklssn
Copy link
Contributor Author

hnrklssn commented Jun 7, 2025

@swift-ci please smoke test macos

@hnrklssn hnrklssn force-pushed the swiftify-inherit-imports branch from 7c794aa to 726d7b3 Compare June 8, 2025 01:04
@hnrklssn
Copy link
Contributor Author

hnrklssn commented Jun 8, 2025

@swift-ci please smoke test macos

@hnrklssn hnrklssn force-pushed the swiftify-inherit-imports branch from 726d7b3 to b3c5489 Compare June 8, 2025 04:27
@hnrklssn
Copy link
Contributor Author

hnrklssn commented Jun 8, 2025

@swift-ci please smoke test macos

hnrklssn added a commit to hnrklssn/llvm-project that referenced this pull request Jun 11, 2025
This assert was reportedly added to "Defensively ensure that
GetExternalDeclStmt protects itself from nested deserialization". Given
that this triggers without nested deserialization we should remove this
assert. The assert blocks swiftlang/swift#81859

rdar://153085264
hnrklssn added a commit to hnrklssn/llvm-project that referenced this pull request Jun 11, 2025
This assert was reportedly added to "Defensively ensure that
GetExternalDeclStmt protects itself from nested deserialization". Given
that this triggers without nested deserialization we should remove this
assert. The assert blocks swiftlang/swift#81859

rdar://153085264
@DougGregor
Copy link
Member

swiftlang/llvm-project#10925

@swift-ci please smoke test

@hnrklssn
Copy link
Contributor Author

hnrklssn commented Jul 8, 2025

swiftlang/llvm-project#10957

@swift-ci please smoke test

@hnrklssn hnrklssn force-pushed the swiftify-inherit-imports branch from b3c5489 to c01392c Compare July 9, 2025 07:52
@hnrklssn
Copy link
Contributor Author

hnrklssn commented Jul 9, 2025

swiftlang/llvm-project#10957

@swift-ci please smoke test

@hnrklssn
Copy link
Contributor Author

@swift-ci please smoke test

hnrklssn added 4 commits July 11, 2025 17:31
When macros like _SwiftifyImport are added to a wrapper module for a
clang module, they may need to refer to symbols declared in another
clang module that the wrapped module imports (e.g. because they are used
in the original signature). This adds all the imported clang modules as
implicit imports to the wrapper module.

rdar://151611573
@hnrklssn hnrklssn force-pushed the swiftify-inherit-imports branch from c01392c to b2084b2 Compare July 12, 2025 06:52
@hnrklssn
Copy link
Contributor Author

@swift-ci please smoke test

@hnrklssn
Copy link
Contributor Author

swiftlang/llvm-project#10957
@swift-ci please smoke test

@hnrklssn
Copy link
Contributor Author

swiftlang/llvm-project@ffa75c8

@swift-ci please smoke test

@hnrklssn
Copy link
Contributor Author

swiftlang/llvm-project#11022
@swift-ci please smoke test

hnrklssn added 2 commits July 26, 2025 00:35
Decls in Swift wrapper module may not originate in the top-level clang
module with the same name, since decls in clang submodules are dumped
into the top-level module. This is because Swift has no concept of
submodules. To make sure that any imported decl has access to the same
symbols as the original clang decl had, all transitive submodules, and
their imports, are added as implicit imports of the wrapper module. This
is necessary in the case where a submodule is marked `explicit`.

The content in explicit submodules isn't normally made visible when
importing the parent module. Decls from explicit submodules still
end up in the top-level wrapper module however, so in Swift they do
still need to be visible from the top-level module. This is relevant
for macro expansions, so that they can refer to the same types as the
original decl.
@hnrklssn
Copy link
Contributor Author

swiftlang/llvm-project#11022
@swift-ci please smoke test

@hnrklssn hnrklssn requested a review from Xazax-hun July 26, 2025 07:51
Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

This direction looks good, but please implement the two small optimizations I suggest.

llvm::SmallVector<const clang::Module *, 32> SubmoduleWorklist;
SubmoduleWorklist.push_back(underlying);
while (!SubmoduleWorklist.empty()) {
const clang::Module *CurrModule = SubmoduleWorklist.pop_back_val();
Copy link
Member

Choose a reason for hiding this comment

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

Can you put in a set of already-visited modules so we don't re-explore the same subgraph many times?

I->getFullModuleName();
ImportPath::Builder importPath(SwiftContext, swiftModuleName, '.');
UnloadedImportedModule importedModule(importPath.copyTo(SwiftContext), ImportKind::Module);
implicitImportInfo.AdditionalUnloadedImports.push_back(importedModule);
Copy link
Member

Choose a reason for hiding this comment

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

Can we restrict ourselves to only recording the explicit submodules we find rather than recording every submodule we find?

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