Skip to content

Cross import overlay tooling support fixes #30758

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

Jump to bottom
Merged
merged 2 commits into from
Apr 4, 2020
Merged

Cross import overlay tooling support fixes #30758

merged 2 commits into from
Apr 4, 2020

Conversation

nathawes
Copy link
Contributor

@nathawes nathawes commented Apr 1, 2020

Removes the existing SourceFile and ModuleDecl APIs to map an overlay module decl back to its underlying module and adds a simpler getUnderlyingModule() API to ModuleDecl. Updates code completion, cursor info, doc info, interface generation and indexing to use it.

Also fixes:

  • interface generation always printing the synthesized comment listing the required bystander modules that need to be imported for the symbol to be available even if the decl it's associated with wasn't actually printed (e.g. due to failed availability checks)
  • code completion after a qualifying module name to also include symbols from any of its shadowing overlays.

Resolves rdar://problem/59445352

@nathawes nathawes requested review from akyrtzi and beccadax April 1, 2020 19:50
@nathawes
Copy link
Contributor Author

nathawes commented Apr 1, 2020

@swift-ci please test

@swift-ci
Copy link
Contributor

swift-ci commented Apr 1, 2020

Build failed
Swift Test Linux Platform
Git Sha - 1d7a2387cc239db49d7d07e481c624cb7cddc3fe

@akyrtzi
Copy link
Contributor

akyrtzi commented Apr 1, 2020

I'm wondering whether it would be beneficial to avoid using the same term "underlying module" in relation to either cross-import overlays or Swift overlays that hide a Clang module. Would it be better if we choose a different term for cross-import overlays, like e.g. "primary"?

Also consider with this PR we'll have ModuleDecl::getUnderlyingModule() which only applies to cross-import overlays and ModuleDecl::findUnderlyingClangModule() that applies for Swift overlays.
It can be hard to remember what ModuleDecl::getUnderlyingModule() is about when just reading some code, I'd suggest to at least include "CrossImport" in all APIs and fields that are related to cross-import overlays so that it is clear when reading code (so change it to ModuleDecl::getUnderlyingModuleOfCrossImportOvelay(), MapCrossImportOverlayModulesToUnderlying, etc).

@akyrtzi
Copy link
Contributor

akyrtzi commented Apr 1, 2020 •
edited
Loading

I'd suggest to at least include "CrossImport" in all APIs and fields

To clarify, I'm suggesting to do this regardless if we keep using "underlying" for cross-import overlays or something else.

@nathawes
Copy link
Contributor Author

nathawes commented Apr 1, 2020

"primary" and "secondary" were used in the pitch, but they only show up in a single doc comment from what I can tell. Most of the code and comments use "underlying" or "shadowed" for "primary" and "bystander" for "secondary".

@akyrtzi What about "declaring" to match the "declared" APIs in ModuleDecl? i.e. ModuleDecl::getDeclaringModuleIfCrossImportOverlay(), to match the existing ModuleDecl::findDeclaredCrossImportOverlays()

@akyrtzi
Copy link
Contributor

akyrtzi commented Apr 1, 2020

@akyrtzi What about "declaring" to match the "declared" APIs in ModuleDecl?

SGTM.

@nathawes nathawes force-pushed the cross-import-fixes branch from 1d7a238 to 058ecdd Compare April 2, 2020 04:04
@nathawes
Copy link
Contributor Author

nathawes commented Apr 2, 2020

@swift-ci please test

@swift-ci
Copy link
Contributor

swift-ci commented Apr 2, 2020

Build failed
Swift Test Linux Platform
Git Sha - 1d7a2387cc239db49d7d07e481c624cb7cddc3fe

@swift-ci
Copy link
Contributor

swift-ci commented Apr 2, 2020

Build failed
Swift Test OS X Platform
Git Sha - 1d7a2387cc239db49d7d07e481c624cb7cddc3fe

@nathawes
Copy link
Contributor Author

nathawes commented Apr 2, 2020

@swift-ci please smoke test Linux

@beccadax
Copy link
Contributor

beccadax commented Apr 2, 2020

Yes, "declaring" is a good term to use here.

beccadax
beccadax approved these changes Apr 2, 2020
View reviewed changes
Copy link
Contributor

@beccadax beccadax left a comment

Choose a reason for hiding this comment

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

Just a few nits; otherwise, LGTM.

include/swift/AST/Module.h Outdated
/// either directly or transitively, populates \p bystanderNames with the set
/// of bystander modules that must be present alongside \p declaring for
/// the overlay to be imported and returns true. Returns false otherwise.
bool getRequiredByStandersIfCrossImportOverlay(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Bystanders, not ByStanders.

lib/AST/Module.cpp Outdated
return declaredCrossImportsTransitive.find(overlay) !=
declaredCrossImportsTransitive.end();
}
if (!getNameStr().startswith("_"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Should you use Decl::hasUnderscoredNaming() here?

(I think there are a few other places you could do that, too.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh thanks! Didn't know about that one.

lib/AST/Module.cpp Outdated
SmallVector<ModuleDecl::ImportedModule, 16> furtherImported;
ModuleDecl *overlayModule = this;

// FIXME: safe to assume the underlying module of an overlay is exported?
Copy link
Contributor

Choose a reason for hiding this comment

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

All well-formed cross-import overlays should @_exported import their declaring module; there are some examples in the test suite that don't, but that's just to make certain tests easier to write. So you can update this comment to explain that.

Nathan Hawes added 2 commits April 3, 2020 16:04
Add method to ModuleDecl to lazily compute the underlying module for …
9f6f763 
…cross-import overlays

Also update code completion, indexing, interface generation and doc info to use it.
[IDE] Completion after a qualifying module should return symbols from…
1a65b84 
… its shadowing cross-import overlays.
@nathawes nathawes force-pushed the cross-import-fixes branch from 058ecdd to 1a65b84 Compare April 3, 2020 23:04
@nathawes
Copy link
Contributor Author

nathawes commented Apr 3, 2020

@swift-ci please test and merge

@nathawes
Copy link
Contributor Author

nathawes commented Apr 4, 2020

@swift-ci test and merge

@swift-ci swift-ci merged commit 51a2a07 into swiftlang:master Apr 4, 2020
@nathawes nathawes deleted the cross-import-fixes branch April 4, 2020 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akyrtzi akyrtzi akyrtzi approved these changes

@beccadax beccadax beccadax approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nathawes @swift-ci @akyrtzi @beccadax