[SymbolGraphGen] Emit symbols from exported modules #41577
Conversation
franklinsch
force-pushed
the
emit-exported-symbols
branch
3 times, most recently
from
acb4d2f to
1932c50
Compare
|
@swift-ci please test |
|
@swift-ci please build toolchain macOS platform |
franklinsch
changed the title
|
@swift-ci please test macOS platform |
There was a problem hiding this comment.
Added this test to verify the during-the-build behaviour didn't regress.
include/swift/AST/Module.h
Outdated
There was a problem hiding this comment.
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 SourceFiles. The ones seen by swift-symbolgraph-extract (and during the build on an -emit-module build) are all going to be SerializedASTFiles, 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 SourceFiles.
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.
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.
lib/SymbolGraphGen/SymbolGraph.cpp
Outdated
There was a problem hiding this comment.
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.
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. |
franklinsch
force-pushed
the
emit-exported-symbols
branch
from
1932c50 to
81fe566
Compare
There was a problem hiding this comment.
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.
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
franklinsch
force-pushed
the
emit-exported-symbols
branch
from
81fe566 to
9cd44ca
Compare
|
@swift-ci Please test |
| } | ||
|
|
||
| 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.
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.
Note: Daniel and I discussed this further and opted to keep the code as-is to align with other functions of this type.
| using namespace symbolgraphgen; | ||
|
|
||
| SymbolGraphASTWalker::SymbolGraphASTWalker(ModuleDecl &M, | ||
| const SmallPtrSet<ModuleDecl *, 4> ExportedImportedModules, |
There was a problem hiding this comment.
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.
QuietMisdreavus
left a comment
QuietMisdreavus
left a comment
There was a problem hiding this comment.
I think this is good enough to land for now. We can work on improving it later.
3 participants
Resolves SR-15753 / rdar://89547374
Emit symbols that come from
@_exported imports.SymbolGraphGenwas 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
SymbolGraphASTWalkerkeep 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 imports 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.