-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[SymbolGraphGen] Emit symbols from exported modules #41577
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
Conversation
acb4d2f
to
1932c50
Compare
@swift-ci please test |
@swift-ci please build toolchain macOS platform |
@swift-ci please test macOS platform |
|
||
// RUN: %empty-directory(%t) | ||
// RUN: %target-build-swift %s -module-name BasicExtension -emit-module -emit-module-path %t/ -emit-symbol-graph -emit-symbol-graph-dir %t | ||
// RUN: %FileCheck %s --input-file %t/[email protected] --check-prefix BUILD |
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.
Added this test to verify the during-the-build behaviour didn't regress.
@@ -939,6 +939,9 @@ inline SourceLoc extractNearestSourceLoc(const ModuleDecl *mod) { | |||
return extractNearestSourceLoc(static_cast<const Decl *>(mod)); | |||
} | |||
|
|||
/// Collects modules that this module imports via `@_exported import`. | |||
void collectParsedExportedImports(const ModuleDecl *M, SmallPtrSetImpl<ModuleDecl *> &Imports); |
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'm willing to bet that this is the cause of SR-15921. This function only works on parsed modules, i.e. ones where the files are all SourceFile
s. The ones seen by swift-symbolgraph-extract
(and during the build on an -emit-module
build) are all going to be SerializedASTFile
s, which won't work with this function. We may need to fall back to a manual decl crawl for modules which aren't parsed directly from SourceFile
s.
Leaving this as-is may suit our purposes now, but i feel like having multiple entry points to "get a symbol graph" that have different behaviors will bite us later. If it's possible to add it to this PR, i'd love to see it.
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, interesting. I'd have to look more into that. I'd prefer if we could land this as-is and look into that issue separately as this unblocks me for what I'm trying to do.
@@ -674,12 +674,6 @@ bool SymbolGraph::isUnconditionallyUnavailableOnAllPlatforms(const Decl *D) cons | |||
|
|||
/// Returns `true` if the symbol should be included as a node in the graph. | |||
bool SymbolGraph::canIncludeDeclAsNode(const Decl *D) const { | |||
// If this decl isn't in this module, don't record it, | |||
// as it will appear elsewhere in its module's symbol graph. | |||
if (D->getModuleContext()->getName() != M.getName()) { |
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.
Something feels off about removing this change entirely. It's probably fine as-is, but i wonder if we could have the SymbolGraph
type get the set of exported-import modules and have a version of isFromExportedImportedModule
like you wrote on the AST Walker? That way we can be thorough.
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.
Yep, I agree. I added that check back with a isFromExportedImportedModule
check.
The implementation itself looks good, but i'd like to either see my comments resolved or some manual testing done before i give this a proper green check. |
1932c50
to
81fe566
Compare
|
||
@_exported import A | ||
|
||
// CHECK: "precise":"s:1A11SymbolFromAV" |
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 think it might also be worth adding a test that makes sure that [email protected]
or the like doesn't get generated as a result of the module-inclusion rules getting relaxed. Something like:
// RUN: ls %t | %FileCheck --check-prefix FILES
...
// FILES-NOT: [email protected]
Again, mainly just me being overly paranoid 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.
No, that's a great idea 👍 Added.
When emitting a symbol graph file for a module that import modules via `@_exported import`, emits those modules' symbols as well. SR-15753 rdar://89547374
81fe566
to
9cd44ca
Compare
@swift-ci Please test |
@@ -788,7 +788,7 @@ bool ModuleDecl::shouldCollectDisplayDecls() const { | |||
return true; | |||
} | |||
|
|||
static void collectParsedExportedImports(const ModuleDecl *M, SmallPtrSetImpl<ModuleDecl *> &Imports) { | |||
void swift::collectParsedExportedImports(const ModuleDecl *M, SmallPtrSetImpl<ModuleDecl *> &Imports) { |
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.
Imports is just returned data so consider returning it directly (RVO means you won't incur the extra copy). However, having had a quick look the usage pattern for the Imports set seems to be very much a case of doing the insertions here and the clients only querying the set. You might be better off using the sorted vector approach described here https://llvm.org/docs/ProgrammersManual.html#id64
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.
Note: Daniel and I discussed this further and opted to keep the code as-is to align with other functions of this type.
@@ -22,9 +22,11 @@ using namespace swift; | |||
using namespace symbolgraphgen; | |||
|
|||
SymbolGraphASTWalker::SymbolGraphASTWalker(ModuleDecl &M, | |||
const SmallPtrSet<ModuleDecl *, 4> ExportedImportedModules, |
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.
If you use the sorted vector approach described in the comment on collectParsedExportedImports
you won't have to hardcode the small size template parameter here. You can then pass the data via SmallVectorImpl
or an ArrayRef
. You would need to change the definition of isFromExportedImportedModule
to use std::lower_bound
to perform the binary search.
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 think this is good enough to land for now. We can work on improving it later.
Resolves SR-15753 / rdar://89547374
Emit symbols that come from
@_exported import
s.SymbolGraphGen
was already receiving these symbols as part of its call togetTopLevelDeclsForDisplay
, but was then dropping them because of subsequent filtering logic. Specifically, we was dropping them because the name of their defining module didn't match the name of main module we're emitting symbol graph files for.To resolve this, I made
SymbolGraphASTWalker
keep track of the module's exported imports so that the symbol filtering logic can check whether the symbol comes from an exported import, and if so, emit that symbol into the main graph.Note: for some reason, it looks like the symbols from
@_exported import
s are not coming through when usingswift-symbolgraph-extract
. They're only coming through when emitting SGFs as during the build. I filed https://bugs.swift.org/browse/SR-15921 to track this.