Skip to content

Swift: extract ImportDecl and ModuleDecl #9772

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 6 commits into from
Jul 1, 2022
Merged

Conversation

redsun82
Copy link
Contributor

@redsun82 redsun82 commented Jul 1, 2022

As ASTMangler crashes when called on ModuleDecl, we simply use
its name.

This might probably not work reliably in a scenario where multiple
modules are compiled with the same name (like main), but this is left
for future work. At the moment this cannot create DB inconsistencies.

The current module is explicitly passed to be extracted in
SwiftExtractor.cpp.

redsun82 added 3 commits July 1, 2022 15:22
As `ASTMangler` crashes when called on `ModuleDecl`, we simply use
its name.

This might probably not work reliably in a scenario where multiple
modules are compiled with the same name (like `main`), but this is left
for future work. At the moment this cannot create DB inconsistencies.
@redsun82 redsun82 requested review from a team as code owners July 1, 2022 13:43
@redsun82 redsun82 requested a review from AlexDenisov July 1, 2022 13:43
@redsun82 redsun82 removed the request for review from a team July 1, 2022 13:45
@github-actions github-actions bot removed the Java label Jul 1, 2022
@redsun82 redsun82 added the no-change-note-required This PR does not need a change note label Jul 1, 2022
Copy link
Contributor

@AlexDenisov AlexDenisov left a comment

Choose a reason for hiding this comment

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

LGTM, with one question: does deduplication affect the module emission? We have different modules with the same name (e.g. Foundation coming from Foundation.swiftinterface and Foundation.pcm).

// TODO the following will be moved to the dispatcher when we start caching swift file objects
// for the moment, topLevelDecls always contains the current module, which does not have a file
// associated with it, so we need a special case when there are no top level declarations
if (topLevelDecls.size() == 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess there is also no harm to emit the file unconditionally? 🤔
(Just thinking out loud, no action needed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, my idea is that we would just unconditionally call visitor.extract(file), which would contain the logic for the emission and caching like the rest of the entities.

out_ << "// DEBUG: ";
(out_ << ... << args) << '\n';
out_ << "/* DEBUG:\n";
(out_ << ... << args) << "\n*/\n";
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uh, I forgot to mention the debugging enhancements. There's also emitDebugInfo on the SwiftDispatcher now, that forwards to trap.debug

// TODO find a more unique string working also when different modules are compiled with the same
// name
if (decl.getKind() == swift::DeclKind::Module) {
return static_cast<const swift::ModuleDecl&>(decl).getRealName().str().str();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is my understanding correct that this is triggered only from the imports?

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, not only, because I also added explicitly the current module ModuleDecl to the top-level declarations in SwiftExtractor.cpp

@redsun82
Copy link
Contributor Author

redsun82 commented Jul 1, 2022

LGTM, with one question: does deduplication affect the module emission? We have different modules with the same name (e.g. Foundation coming from Foundation.swiftinterface and Foundation.pcm).

This is a case of imperfect deduplication, yes. It gets worse as the SPM will build lots of different sources with -module-name main.

For the moment this is not a problem for the DB, as the entries only have limited information (the name and the builtin/system predicates), but this definitely needs to be solved at some point, as part of the TODO removal initiative 🙂

@redsun82 redsun82 merged commit 563d273 into main Jul 1, 2022
@redsun82 redsun82 deleted the redsun82/swift-extraction branch July 1, 2022 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note Swift
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants