Skip to content

[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

Merged
merged 1 commit into from
Mar 2, 2022

Conversation

franklinsch
Copy link
Contributor

@franklinsch franklinsch commented Feb 26, 2022

Resolves SR-15753 / rdar://89547374

Emit symbols that come from @_exported imports. SymbolGraphGen was already receiving these symbols as part of its call to getTopLevelDeclsForDisplay, 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 imports are not coming through when using swift-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.

@franklinsch franklinsch force-pushed the emit-exported-symbols branch 3 times, most recently from acb4d2f to 1932c50 Compare February 28, 2022 09:48
@franklinsch
Copy link
Contributor Author

@swift-ci please test

@franklinsch
Copy link
Contributor Author

@swift-ci please build toolchain macOS platform

@franklinsch franklinsch changed the title [WIP] [SymbolGraphGen] Emit symbols from exported modules [SymbolGraphGen] Emit symbols from exported modules Feb 28, 2022
@franklinsch
Copy link
Contributor Author

@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
Copy link
Contributor Author

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);
Copy link
Contributor

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 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.

Copy link
Contributor Author

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()) {
Copy link
Contributor

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.

Copy link
Contributor Author

@franklinsch franklinsch Feb 28, 2022

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.

@QuietMisdreavus
Copy link
Contributor

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 franklinsch force-pushed the emit-exported-symbols branch from 1932c50 to 81fe566 Compare February 28, 2022 17:14

@_exported import A

// CHECK: "precise":"s:1A11SymbolFromAV"
Copy link
Contributor

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. 😅

Copy link
Contributor Author

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
@franklinsch franklinsch force-pushed the emit-exported-symbols branch from 81fe566 to 9cd44ca Compare February 28, 2022 17:21
@QuietMisdreavus
Copy link
Contributor

@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) {
Copy link
Contributor

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

Copy link
Contributor Author

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,
Copy link
Contributor

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.

Copy link
Contributor

@QuietMisdreavus QuietMisdreavus left a 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.

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