Skip to content
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

[CodeCompletion] Complete Swift only module name after 'import' #24455

Merged
merged 6 commits into from May 8, 2019

Conversation

Projects
None yet
5 participants
@rintaro
Copy link
Member

commented May 3, 2019

For each ModuleLoader, newly implement collectVisibleTopLevelModuleNames() which collects visible module names. It doesn't care about the actual content of the file though. So it doesn't guarantee the module is loadable.

rdar://problem/39392446

@rintaro rintaro force-pushed the rintaro:ide-complete-import-swiftmodule branch 4 times, most recently from e3bfbc0 to 3f3ff69 May 3, 2019

@rintaro

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

@swift-ci Please smoke test

@rintaro rintaro marked this pull request as ready for review May 3, 2019

@rintaro rintaro requested review from benlangmuir and jrose-apple May 3, 2019

if (ModuleName.str().startswith("_"))
continue;
if (ModuleName == Ctx.SwiftShimsModuleName)
continue;

This comment has been minimized.

Copy link
@rintaro

rintaro May 3, 2019

Author Member

Are there other module names we should hide? e.g. SwiftOnoneSupport

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple May 6, 2019

Member

Yeah, that one seems worth hiding. That's probably all we want to hardcode in the tools.

This comment has been minimized.

Copy link
@benlangmuir

benlangmuir May 6, 2019

Member

+1 to skipping OnoneSupport (looks like the name is in Strings.h). Other modules we should skip: __C, __C_Synthesized, Builtin. I'm guessing they are already hidden because they're not on the file system, but please add CHECK-NOTs for them.

This comment has been minimized.

Copy link
@brentdax

brentdax May 6, 2019

Collaborator

_InternalSwiftSyntaxParser is another one we don't want to show. Should already be covered, but again, worth a test to make sure.

}
case SearchPathKind::Framework: {
// Look for:
// $PATH/{name}.framework/Modules/{name}.swiftmodule/{arch}.{extension}

This comment has been minimized.

Copy link
@rintaro

rintaro May 3, 2019

Author Member

Not sure we need to handle frameworks practically. If framework always have Clang module (module.modulemap) with corresponding module name, ClangImporter can find them.
@jrose-apple Is pure Swift only framework possible?

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple May 6, 2019

Member

Yes, unfortunately.

@rintaro rintaro force-pushed the rintaro:ide-complete-import-swiftmodule branch 2 times, most recently from fb5c6f3 to 3b099bb May 3, 2019

@rintaro

This comment has been minimized.

Copy link
Member Author

commented May 4, 2019

@swift-ci Please test

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

Build failed
Swift Test OS X Platform
Git Sha - 3f3ff69

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

Build failed
Swift Test Linux Platform
Git Sha - 3f3ff69

@rintaro

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

@swift-ci Please test

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

Build failed
Swift Test Linux Platform
Git Sha - 3b099bb

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

Build failed
Swift Test OS X Platform
Git Sha - 3b099bb

auto MD = ModuleDecl::create(Ctx.getIdentifier(ModuleName), Ctx);
if (ModuleName == mainModuleName)
continue;
if (ModuleName.str().startswith("_"))

This comment has been minimized.

Copy link
@benlangmuir

benlangmuir May 6, 2019

Member

I'm not happy about this case, but since it's just been copied okay...

if (ModuleName.str().startswith("_"))
continue;
if (ModuleName == Ctx.SwiftShimsModuleName)
continue;

This comment has been minimized.

Copy link
@benlangmuir

benlangmuir May 6, 2019

Member

+1 to skipping OnoneSupport (looks like the name is in Strings.h). Other modules we should skip: __C, __C_Synthesized, Builtin. I'm guessing they are already hidden because they're not on the file system, but please add CHECK-NOTs for them.


// Check whether target specific module file exists or not in given directory.
// $PATH/{arch}.{extension}
auto checkTargetFiles = [&](StringRef path) -> bool {

This comment has been minimized.

Copy link
@benlangmuir

benlangmuir May 6, 2019

Member

Is there any point in checking these instead of just detecting the presence of the .swiftmodule directory/file? We aren't checking if the module is importable.

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple May 6, 2019

Member

That'd be faster but might mean less common code between both code paths.

This comment has been minimized.

Copy link
@benlangmuir

benlangmuir May 6, 2019

Member

Good point. I'm okay with this if it's not a big perf issue and keeps the code simpler.

@jrose-apple
Copy link
Member

left a comment

I haven't taken a close look at the refactoring for SerializedModuleLoaderBase yet, but here's some initial comments. (Seems intricate…)

if (ModuleName.str().startswith("_"))
continue;
if (ModuleName == Ctx.SwiftShimsModuleName)
continue;

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple May 6, 2019

Member

Yeah, that one seems worth hiding. That's probably all we want to hardcode in the tools.

@@ -70,6 +70,11 @@ class SkipNonTransparentFunctions : public DelayedParsingCallbacks {

} // unnamed namespace

void SourceLoader::collectVisibleTopLevelModuleNames(
SmallVectorImpl<Identifier> &names) const {
// TODO: Implement?

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple May 6, 2019

Member

Nah, SourceLoader's going away soon. (As soon as I can convert the tests over to using module interfaces.)


/// Apply \c body for each target-specific module file base name to search.
void forEachTargetModuleBasename(const ASTContext &Ctx,
llvm::function_ref<void(StringRef)> body) {

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple May 6, 2019

Member

Nitpick: LLVM style is to double-indent the wrapped line (and then single-indent the following lines), and I think we consistently wrap all parameters if any of them can't be aligned to the open paren.

This comment has been minimized.

Copy link
@rintaro

rintaro May 6, 2019

Author Member

Ah, I forgot to apply clang-format to this block. Thanks!

}
case SearchPathKind::Framework: {
// Look for:
// $PATH/{name}.framework/Modules/{name}.swiftmodule/{arch}.{extension}

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple May 6, 2019

Member

Yes, unfortunately.

@jrose-apple jrose-apple requested a review from brentdax May 6, 2019

@brentdax

This comment has been minimized.

Copy link
Collaborator

commented May 6, 2019

Initial reaction is that I like the module loading refactoring quite a bit, but I'll need to study it more to see if it preserves behavior.

@rintaro rintaro force-pushed the rintaro:ide-complete-import-swiftmodule branch from cce47c6 to 9b83c3e May 6, 2019

@brentdax
Copy link
Collaborator

left a comment

The SerializedModuleLoaderBase refactoring is so nice. I knew there was a small, sensible function struggling to escape from that mess.

return result;

// Apple platforms have extra implicit framework search paths:
// $SDKROOT/System/Library/Frameworks/ ; and

This comment has been minimized.

Copy link
@brentdax

brentdax May 6, 2019

Collaborator

This comment seems to have picked up a semicolon in the move.

// Apply \p body for each directory entry in \p dirPath.
auto forEachDirectoryEntryPath =
[&](StringRef dirPath, llvm::function_ref<void(StringRef)> body) {
std::error_code errorCode;

This comment has been minimized.

Copy link
@brentdax

brentdax May 6, 2019

Collaborator

I suppose you're intentionally ignoring the error code here?

This comment has been minimized.

Copy link
@rintaro

rintaro May 6, 2019

Author Member

errorCode is handled in for condition. for (; !errorCode && DI != End;...

// Look for:
// (Darwin OS) $PATH/{name}.swiftmodule/{arch}.{extension}
// (Other OS) $PATH/{name}.{extension}
bool requireTargetSpecificModule = Ctx.LangOpts.Target.isOSDarwin();

This comment has been minimized.

Copy link
@brentdax

brentdax May 6, 2019

Collaborator

This isOSDarwin() test for RuntimeLibrary is written twice. Could we centralize it somehow? Maybe by splitting RuntimeLibrary into TargetSpecificRuntimeLibrary and TargetIndependentRuntimeLibrary?

Alternatively, maybe forEachModuleSearchPath() could pass additional parameters with flags for "can be target independent" and "can be target dependent" (or a single tri-state value). That might allow you to unify more of the framework and non-framework paths in these functions.

This comment has been minimized.

Copy link
@rintaro

rintaro May 6, 2019

Author Member

That seems to be a good idea. Let me try it in follow-ups.

@@ -228,6 +401,7 @@ SerializedModuleLoaderBase::findModule(AccessPathElem moduleID,

// We can only get here if all targetFileNamePairs failed with
// 'std::errc::no_such_file_or_directory'.
auto normalizedTarget = getTargetSpecificModuleTriple(Ctx.LangOpts.Target);

This comment has been minimized.

Copy link
@brentdax

brentdax May 6, 2019

Collaborator

We're going to recompute this pretty often if it's inside the lambda, but I guess it'll be swamped by I/O anyway.

This comment has been minimized.

Copy link
@rintaro

rintaro May 6, 2019

Author Member

This is diagnostics code path. I don't expect this code is going to be executed so often. Am I missing something?

This comment has been minimized.

Copy link
@brentdax

brentdax May 7, 2019

Collaborator

If we end up at the return None; after this, we may still successfully load a module from a later search path. But I guess that only happens if there's a .swiftmodule directory and the directory doesn't contain any accessible .swiftmodule files, so it's not likely to happen repeatedly.

(Hmm, I don't think failed #if canImport(...) results are cached anywhere...unrelated, just something I noticed.)

This comment has been minimized.

Copy link
@rintaro

rintaro May 7, 2019

Author Member

Ah, that's true. Thank you for explanation!

This comment has been minimized.

Copy link
@rintaro

rintaro May 8, 2019

Author Member

Decided to use the first name from forEachTargetModuleBasename()

if (ModuleName.str().startswith("_"))
continue;
if (ModuleName == Ctx.SwiftShimsModuleName)
continue;

This comment has been minimized.

Copy link
@brentdax

brentdax May 6, 2019

Collaborator

_InternalSwiftSyntaxParser is another one we don't want to show. Should already be covered, but again, worth a test to make sure.

@rintaro rintaro force-pushed the rintaro:ide-complete-import-swiftmodule branch from 1d317fd to 334685b May 6, 2019

@rintaro

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

@swift-ci Please smoke test

1 similar comment
@rintaro

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

@swift-ci Please smoke test

@rintaro rintaro force-pushed the rintaro:ide-complete-import-swiftmodule branch from 3fd99cf to 334685b May 7, 2019

rintaro added some commits Apr 23, 2018

[Serialize] Consolidate duplicated logics
- forEachTargetModuleBasename(): Iterate through target names to search
- forEachModuleSearchPath(): Iterate through search paths

@rintaro rintaro force-pushed the rintaro:ide-complete-import-swiftmodule branch from 334685b to d02f170 May 8, 2019

@rintaro

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

@swift-ci Please smoke test

[Serialization] Avoid to call getTargetSpecificModuleTriple() twice
Maintain an invariant that forEachTargetModuleBasename() iterates names
from most to least desiable.

@rintaro rintaro force-pushed the rintaro:ide-complete-import-swiftmodule branch from d02f170 to 0d43e5a May 8, 2019

@rintaro

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

@swift-ci Please smoke test

@rintaro rintaro merged commit 558cc63 into apple:master May 8, 2019

2 checks passed

Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform (smoke test)
Details

@rintaro rintaro deleted the rintaro:ide-complete-import-swiftmodule branch May 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.