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

rintaro · 2019-05-03T00:11:41Z

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 · 2019-05-03T22:04:05Z

@swift-ci Please smoke test

rintaro · 2019-05-03T22:06:51Z

lib/IDE/CodeCompletion.cpp

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


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

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

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

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

rintaro · 2019-05-03T22:12:46Z

lib/Serialization/SerializedModuleLoader.cpp

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


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?

Yes, unfortunately.

rintaro · 2019-05-04T00:04:36Z

@swift-ci Please test

swift-ci · 2019-05-04T00:08:08Z

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

swift-ci · 2019-05-04T00:34:34Z

Build failed
Swift Test Linux Platform
Git Sha - 3f3ff69e79c2b4ce3539eb4f21b6e72e13e4381a

rintaro · 2019-05-06T07:38:36Z

@swift-ci Please test

swift-ci · 2019-05-06T07:40:38Z

Build failed
Swift Test Linux Platform
Git Sha - 3b099bbc8d766f8d41eb800931737ba24c1d64ba

swift-ci · 2019-05-06T07:42:06Z

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

benlangmuir · 2019-05-06T16:15:05Z

lib/IDE/CodeCompletion.cpp

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


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

benlangmuir · 2019-05-06T16:24:13Z

lib/IDE/CodeCompletion.cpp

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


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

benlangmuir · 2019-05-06T16:36:13Z

lib/Serialization/SerializedModuleLoader.cpp

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


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.

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

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

jrose-apple

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

jrose-apple · 2019-05-04T00:53:51Z

lib/IDE/CodeCompletion.cpp

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


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

jrose-apple · 2019-05-04T00:54:10Z

lib/Sema/SourceLoader.cpp

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

 } // unnamed namespace

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


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

jrose-apple · 2019-05-04T00:57:18Z

lib/Serialization/SerializedModuleLoader.cpp

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


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.

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

jrose-apple · 2019-05-04T00:59:01Z

lib/Serialization/SerializedModuleLoader.cpp

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


Yes, unfortunately.

beccadax · 2019-05-06T18:34:36Z

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.

beccadax

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

beccadax · 2019-05-06T20:19:14Z

lib/Serialization/SerializedModuleLoader.cpp

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


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

beccadax · 2019-05-06T20:21:23Z

lib/Serialization/SerializedModuleLoader.cpp

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


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

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

beccadax · 2019-05-06T20:27:25Z

lib/Serialization/SerializedModuleLoader.cpp

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


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.

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

beccadax · 2019-05-06T20:47:01Z

lib/Serialization/SerializedModuleLoader.cpp

@@ -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);


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 is diagnostics code path. I don't expect this code is going to be executed so often. Am I missing something?

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

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

Decided to use the first name from forEachTargetModuleBasename()

beccadax · 2019-05-06T21:02:41Z

lib/IDE/CodeCompletion.cpp

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


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

rintaro · 2019-05-06T22:37:07Z

@swift-ci Please smoke test

rintaro · 2019-05-06T23:05:18Z

@swift-ci Please smoke test

rdar://problem/39392446

- forEachTargetModuleBasename(): Iterate through target names to search - forEachModuleSearchPath(): Iterate through search paths

rintaro · 2019-05-08T17:24:17Z

@swift-ci Please smoke test

Maintain an invariant that forEachTargetModuleBasename() iterates names from most to least desiable.

rintaro · 2019-05-08T19:42:44Z

@swift-ci Please smoke test

jrose-apple · 2019-06-20T02:46:50Z

*sigh* I think the tri-state Optional<bool> ended up causing rdar://problem/50789839. I'll probably go through and replace it all with an enum.

rintaro · 2019-06-20T17:30:48Z

@jrose-apple Could you elaborate? Did I make a mistake?

jrose-apple · 2019-06-20T17:49:00Z

I'm not sure. This morning I'm thinking about it more and I think you didn't make a mistake, but I'm still taking this as evidence that the code isn't as easy-to-read as it could be.

jrose-apple · 2019-06-20T17:51:55Z

(The issue is that ParseableInterfaceModuleLoader is failing but the compilation process overall is "succeeding", i.e. exiting with an exit code of 0.)

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

rintaro marked this pull request as ready for review May 3, 2019 22:04

rintaro requested review from benlangmuir and jrose-apple May 3, 2019 22:04

rintaro commented May 3, 2019

View reviewed changes

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

benlangmuir reviewed May 6, 2019

View reviewed changes

jrose-apple reviewed May 6, 2019

View reviewed changes

jrose-apple requested a review from beccadax May 6, 2019 17:22

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

beccadax approved these changes May 6, 2019

View reviewed changes

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

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

rintaro added 2 commits May 8, 2019 10:11

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

6956089

rdar://problem/39392446

[Serialize] Consolidate duplicated logics

8773fe3

- forEachTargetModuleBasename(): Iterate through target names to search - forEachModuleSearchPath(): Iterate through search paths

rintaro added 3 commits May 8, 2019 10:11

[test] Add test cases for Swift module completion after 'import'

502f530

[ASTContext] Remove unused function and includes

acd4cb1

[CodeCompletion] Hide 'SwiftOnoneSupport' from module name completion

ffde228

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

[Serialization] Avoid to call getTargetSpecificModuleTriple() twice

0d43e5a

Maintain an invariant that forEachTargetModuleBasename() iterates names from most to least desiable.

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

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

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

This was referenced May 9, 2019

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

Merged

[5.1-04-24-2019][CodeCompletion] Complete Swift only module name after 'import' #24643

Merged

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

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

Uh oh!

Conversation

rintaro commented May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rintaro commented May 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rintaro commented May 4, 2019

Uh oh!

swift-ci commented May 4, 2019

Uh oh!

swift-ci commented May 4, 2019

Uh oh!

rintaro commented May 6, 2019

Uh oh!

swift-ci commented May 6, 2019

Uh oh!

swift-ci commented May 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beccadax commented May 6, 2019

Uh oh!

beccadax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

rintaro commented May 3, 2019 •

edited

Loading