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.

Sign up for GitHub

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
Merged

[SymbolGraphGen] Emit symbols from exported modules #41577

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions include/swift/AST/Module.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.


} // end namespace swift

#endif
2 changes: 1 addition & 1 deletion lib/AST/Module.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

for (const FileUnit *file : M->getFiles()) {
if (const SourceFile *source = dyn_cast<SourceFile>(file)) {
if (source->hasImports()) {
Expand Down
6 changes: 3 additions & 3 deletions lib/SymbolGraphGen/SymbolGraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -674,12 +674,12 @@ 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,
// If this decl isn't in this module or module that this module imported with `@_exported`, 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 •
edited
Loading

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.

if (D->getModuleContext()->getName() != M.getName() && !Walker.isFromExportedImportedModule(D)) {
return false;
}

if (!isa<ValueDecl>(D)) {
return false;
}
Expand Down
15 changes: 15 additions & 0 deletions lib/SymbolGraphGen/SymbolGraphASTWalker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

const SymbolGraphOptions &Options)
: Options(Options),
M(M),
ExportedImportedModules(ExportedImportedModules),
MainGraph(*this, M, None, Ctx) {}

/// Get a "sub" symbol graph for the parent module of a type that
Expand All @@ -51,6 +53,11 @@ SymbolGraph *SymbolGraphASTWalker::getModuleSymbolGraph(const Decl *D) {
// should put actual extensions of that module into the main graph
return &MainGraph;
}

if (isFromExportedImportedModule(D)) {
return &MainGraph;
}

auto Found = ExtendedModuleGraphs.find(M->getNameStr());
if (Found != ExtendedModuleGraphs.end()) {
return Found->getValue();
Expand Down Expand Up @@ -208,3 +215,11 @@ bool SymbolGraphASTWalker::walkToDeclPre(Decl *D, CharSourceRange Range) {

return true;
}

bool SymbolGraphASTWalker::isFromExportedImportedModule(const Decl* D) const {
auto *M = D->getModuleContext();

return llvm::any_of(ExportedImportedModules, [&M](const auto *MD) {
return M->getNameStr().equals(MD->getModuleContext()->getNameStr());
});
}
11 changes: 10 additions & 1 deletion lib/SymbolGraphGen/SymbolGraphASTWalker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ struct SymbolGraphASTWalker : public SourceEntityWalker {

/// The module that this symbol graph will represent.
const ModuleDecl &M;

const SmallPtrSet<ModuleDecl *, 4> ExportedImportedModules;

/// The symbol graph for the main module of interest.
SymbolGraph MainGraph;
Expand All @@ -55,7 +57,9 @@ struct SymbolGraphASTWalker : public SourceEntityWalker {

// MARK: - Initialization

SymbolGraphASTWalker(ModuleDecl &M, const SymbolGraphOptions &Options);
SymbolGraphASTWalker(ModuleDecl &M,
const SmallPtrSet<ModuleDecl *, 4> ExportedImportedModules,
const SymbolGraphOptions &Options);
virtual ~SymbolGraphASTWalker() {}

// MARK: - Utilities
Expand Down Expand Up @@ -87,6 +91,11 @@ struct SymbolGraphASTWalker : public SourceEntityWalker {
// MARK: - SourceEntityWalker

virtual bool walkToDeclPre(Decl *D, CharSourceRange Range) override;

// MARK: - Utilities

/// Returns whether the given declaration comes from an `@_exported import` module.
virtual bool isFromExportedImportedModule(const Decl *D) const;
};

} // end namespace symbolgraphgen
Expand Down
8 changes: 6 additions & 2 deletions lib/SymbolGraphGen/SymbolGraphGen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,17 @@ int serializeSymbolGraph(SymbolGraph &SG,
int
symbolgraphgen::emitSymbolGraphForModule(ModuleDecl *M,
const SymbolGraphOptions &Options) {
SymbolGraphASTWalker Walker(*M, Options);
SmallVector<Decl *, 64> ModuleDecls;
swift::getTopLevelDeclsForDisplay(M, ModuleDecls, /*recursive*/true);

SmallPtrSet<ModuleDecl *, 4> ExportedImportedModules;
swift::collectParsedExportedImports(M, ExportedImportedModules);

if (Options.PrintMessages)
llvm::errs() << ModuleDecls.size()
<< " top-level declarations in this module.\n";

SymbolGraphASTWalker Walker(*M, ExportedImportedModules, Options);

for (auto *Decl : ModuleDecls) {
Walker.walk(Decl);
Expand Down Expand Up @@ -98,7 +102,7 @@ printSymbolGraphForDecl(const ValueDecl *D, Type BaseTy,

llvm::json::OStream JOS(OS, Options.PrettyPrint ? 2 : 0);
ModuleDecl *MD = D->getModuleContext();
SymbolGraphASTWalker Walker(*MD, Options);
SymbolGraphASTWalker Walker(*MD, {}, Options);
markup::MarkupContext MarkupCtx;
SymbolGraph Graph(Walker, *MD, None, MarkupCtx, None,
/*IsForSingleNode=*/true);
Expand Down
31 changes: 23 additions & 8 deletions test/SymbolGraph/Module/BasicExtension.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
// RUN: %empty-directory(%t)
// RUN: %target-build-swift %s -module-name BasicExtension -emit-module -emit-module-path %t/
// RUN: %target-swift-symbolgraph-extract -module-name BasicExtension -I %t -pretty-print -output-dir %t
// RUN: %FileCheck %s --input-file %t/[email protected]
// RUN: %FileCheck %s --input-file %t/[email protected] --check-prefix EXTRACT

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


extension String {
/// Return something.
Expand All @@ -10,17 +14,28 @@ extension String {
}
}

// CHECK: module
// CHECK-NEXT: "name": "BasicExtension"
// EXTRACT: module
// EXTRACT-NEXT: "name": "BasicExtension"

// BUILD: module
// BUILD: "name":"BasicExtension"

// CHECK: "precise": "s:SS14BasicExtensionE9somethingSSvp"
// EXTRACT: "precise": "s:SS14BasicExtensionE9somethingSSvp"

// CHECK: "kind": "memberOf"
// CHECK-NEXT: "source": "s:SS14BasicExtensionE9somethingSSvp"
// CHECK-NEXT: "target": "s:SS"
// BUILD: "precise":"s:SS14BasicExtensionE9somethingSSvp"

// EXTRACT: "kind": "memberOf"
// EXTRACT-NEXT: "source": "s:SS14BasicExtensionE9somethingSSvp"
// EXTRACT-NEXT: "target": "s:SS"

// BUILD: "kind":"memberOf"
// BUILD: "source":"s:SS14BasicExtensionE9somethingSSvp"
// BUILD: "target":"s:SS"

// Extending `String` creates a memberOf relationship above.
// However, it should not be included as a node because `String`
// is owned by the Swift module.
// rdar://58876107
// CHECK-NOT: "precise": "s:SS"
// EXTRACT-NOT: "precise": "s:SS"

// BUILD-NOT: "precise":"s:SS"
15 changes: 15 additions & 0 deletions test/SymbolGraph/Module/ExportedImport.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// RUN: %empty-directory(%t)
// RUN: %target-swift-frontend %S/Inputs/ExportedImport/A.swift -module-name A -emit-module -emit-module-path %t/A.swiftmodule
// RUN: %target-swift-frontend %s -module-name ExportedImport -emit-module -emit-module-path /dev/null -I %t -emit-symbol-graph -emit-symbol-graph-dir %t/
// RUN: %FileCheck %s --input-file %t/ExportedImport.symbols.json
// RUN: ls %t | %FileCheck %s --check-prefix FILES

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

// CHECK-NOT: InternalSymbolFromA

// FIXME: Symbols from `@_exported import` do not get emitted when using swift-symbolgraph-extract
// This is tracked by https://bugs.swift.org/browse/SR-15921.

// FILES-NOT: [email protected]
3 changes: 3 additions & 0 deletions test/SymbolGraph/Module/Inputs/ExportedImport/A.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
public struct SymbolFromA {}

struct InternalSymbolFromA {}