Support framework import/include auto-completion

[dgoldman]

Summary

Frameworks filesystem representations:

UIKit.framework/Headers/%header%

Framework import format:

#import <UIKit/%header%>

Thus the completion code must map the input format of <UIKit/> to
the path of UIKit.framework/Headers as well as strip the
".framework" suffix when auto-completing the framework name.

[sammccall]

lib/Sema/SemaCodeComplete.cpp

8375

nit: IsFramework, StripFrameworkSuffix etc

8375

probably clearer just to pass in the DirectoryLookup::LookupType_t, rather than another bool.

8377–8378

I don't think keeping the stripFrameworkSuffix state is worth scattering the reader's attention here.

I'd suggest instead:

if (!NativeRelDir.empty()) {
  if (LookupType == LT_Framework) {
    // In a framework directory, Foo/Bar/ is actually Foo.framework/Headers/Bar/.
    ...
  } else {
    append(Dir, NativeRelDir);
  }
}

or even pass Dir, NativeRelDir, and LookupType to a helper function to sort it out.

Below, you can just check NativeRelDir.empty() && type == Framework again...

If you do choose to keep it, please give it a descriptive name rather than an imperative one, e.g. ListingFrameworksDir

8377–8378

this needs some explanation (mostly the why, not the how)

8387

just append(Dir, *Begin + ".framework", "Headers") ?
append takes twines I think.

8389

why is the if/else needed? the code above should cover both cases, the last append is a no-op if ++begin == end.

8407

if a framework directory contains a subdirectory that does not end in ".framework", is it actually legal to include from it? Maybe we should be omitting it.

8408

if (should strip suffix)
  Filename.consume_back(".framework")

(no need for else)

8408

one-line comment should be enough to explain the intent here: // Entries in a framework directory have a .framework suffix, this does not appear in the source code

[dgoldman]

• Misc fixes

[sammccall]

Code looks good apart from one case where we encounter Foo.framework/Subdir1/Subdir2.

Can you add a test to clang/test/CodeCompletion? included-files.cpp has the existing tests, not sure if you can add them there or it needs to be an obj-c file for framework includes.

lib/Sema/SemaCodeComplete.cpp

8404

nit: no {} needed for these one-line ifs

8404

this check appears to be incorrect, it'll fire even if NativeRelDir is nonempty (i.e. a we're inside a framework already.)
I think this can be combined with the one below it, see next comment.

8407

consume_back checks for the suffix, just if (NativeRelDir.empty() && !Filename.consume_back(".framework")) break; is enough

[dgoldman]

• Add test and fix subfolder bug

[dgoldman]

added test

[sammccall]

Great, thank you! Want me to land this?

(You can certainly get your own commit access at this point if you like: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access and point at a couple of your patches)

[dgoldman]

In D58062#1399499, @sammccall wrote:

Great, thank you! Want me to land this?

(You can certainly get your own commit access at this point if you like: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access and point at a couple of your patches)

Sure, you can land this one, I''ll try to get commit access in the meantime.

[dgoldman]

Closed via rL355008

lib/Sema/SemaCodeComplete.cpp

if (R.second) { // New completion
       Builder.AddTypedTextChunk(InternedTyped);
       // The result is a "Pattern", which is pretty opaque.
       // We may want to include the real filename to allow smart ranking.
       Results.AddResult(CodeCompletionResult(Builder.TakeString()));
     }
   };
 
   // Helper: scans IncludeDir for nice files, and adds results for each.
-  auto AddFilesFromIncludeDir = [&](StringRef IncludeDir, bool IsSystem) {
+  auto AddFilesFromIncludeDir = [&](StringRef IncludeDir,
+                                    bool IsSystem,
+                                    DirectoryLookup::LookupType_t LookupType) {
     llvm::SmallString<128> Dir = IncludeDir;
-    if (!NativeRelDir.empty())
+    if (!NativeRelDir.empty()) {
+      if (LookupType == DirectoryLookup::LT_Framework) {
+        // For a framework dir, #include <Foo/Bar/> actually maps to
+        // a path of Foo.framework/Headers/Bar/.
+        auto Begin = llvm::sys::path::begin(NativeRelDir);
+        auto End = llvm::sys::path::end(NativeRelDir);
+
+        llvm::sys::path::append(Dir, *Begin + ".framework", "Headers");
+        llvm::sys::path::append(Dir, ++Begin, End);
+      } else {
         llvm::sys::path::append(Dir, NativeRelDir);
+      }
+    }
 
     std::error_code EC;
     unsigned Count = 0;
     for (auto It = FS->dir_begin(Dir, EC);
          !EC && It != llvm::vfs::directory_iterator(); It.increment(EC)) {
       if (++Count == 2500) // If we happen to hit a huge directory,
         break;             // bail out early so we're not too slow.
       StringRef Filename = llvm::sys::path::filename(It->path());
       switch (It->type()) {
       case llvm::sys::fs::file_type::directory_file:
+        // All entries in a framework directory must have a ".framework" suffix,
+        // but the suffix does not appear in the source code's include/import.
+        if (LookupType == DirectoryLookup::LT_Framework &&
+            NativeRelDir.empty() && !Filename.consume_back(".framework"))
+          break;
+
         AddCompletion(Filename, /*IsDirectory=*/true);
         break;
       case llvm::sys::fs::file_type::regular_file:
         // Only files that really look like headers. (Except in system dirs).
         if (!IsSystem) {
           // Header extensions from Types.def, which we can't depend on here.
           if (!(Filename.endswith_lower(".h") ||
                 Filename.endswith_lower(".hh") ||
                 Filename.endswith_lower(".hpp") ||
                 Filename.endswith_lower(".inc")))
void Sema::CodeCompleteIncludedFile(llvm::StringRef Dir, bool Angled) {
   // Helper: adds results relative to IncludeDir, if possible.
   auto AddFilesFromDirLookup = [&](const DirectoryLookup &IncludeDir,
                                    bool IsSystem) {
     switch (IncludeDir.getLookupType()) {
     case DirectoryLookup::LT_HeaderMap:
       // header maps are not (currently) enumerable.
       break;
     case DirectoryLookup::LT_NormalDir:
-      AddFilesFromIncludeDir(IncludeDir.getDir()->getName(), IsSystem);
+      AddFilesFromIncludeDir(IncludeDir.getDir()->getName(), IsSystem,
+                             DirectoryLookup::LT_NormalDir);
       break;
     case DirectoryLookup::LT_Framework:
-      AddFilesFromIncludeDir(IncludeDir.getFrameworkDir()->getName(), IsSystem);
+      AddFilesFromIncludeDir(IncludeDir.getFrameworkDir()->getName(), IsSystem,
+                             DirectoryLookup::LT_Framework);
       break;
     }
   };
 
   // Finally with all our helpers, we can scan the include path.
   // Do this in standard order so deduplication keeps the right file.
   // (In case we decide to add more details to the results later).
   const auto &S = PP.getHeaderSearchInfo();
   using llvm::make_range;
   if (!Angled) {
     // The current directory is on the include path for "quoted" includes.
     auto *CurFile = PP.getCurrentFileLexer()->getFileEntry();
     if (CurFile && CurFile->getDir())
-      AddFilesFromIncludeDir(CurFile->getDir()->getName(), false);
+      AddFilesFromIncludeDir(CurFile->getDir()->getName(), false,
+                             DirectoryLookup::LT_NormalDir);
     for (const auto &D : make_range(S.quoted_dir_begin(), S.quoted_dir_end()))
       AddFilesFromDirLookup(D, false);
   }
   for (const auto &D : make_range(S.angled_dir_begin(), S.angled_dir_end()))
     AddFilesFromDirLookup(D, false);
   for (const auto &D : make_range(S.system_dir_begin(), S.system_dir_end()))
     AddFilesFromDirLookup(D, true);
 

test/CodeCompletion/included-frameworks.m

+// RUN: rm -rf %t && mkdir -p %t/Foo.framework/Headers/SubFolder && mkdir %t/NotAFramework/
+// RUN: touch %t/Foo.framework/Headers/Foo.h && touch %t/Foo.framework/Headers/FOOClass.h
+// RUN: touch %t/Foo.framework/Headers/SubFolder/FOOInternal.h
+
+#import <Foo/Foo.h>
+
+#import <Foo/SubFolder/FOOInternal.h>
+
+// Note: the run lines follow their respective tests, since line/column
+// matter in this test.
+
+// Autocomplete frameworks without the ".framework" extension.
+//
+// RUN: %clang -fsyntax-only -F %t -Xclang -code-completion-at=%s:5:10 %s -o - | FileCheck -check-prefix=CHECK-1 %s
+// CHECK-1-NOT: Foo.framework/
+// CHECK-1-NOT: NotAFramework/
+// CHECK-1: Foo/
+
+// Autocomplete for frameworks inside its Headers folder.
+//
+// RUN: %clang -fsyntax-only -F %t -Xclang -code-completion-at=%s:5:14 %s -o - | FileCheck -check-prefix=CHECK-2 %s
+// CHECK-2: Foo.h>
+// CHECK-2: FOOClass.h>
+// CHECK-2: SubFolder/
+
+// Autocomplete for folders inside of a frameworks.
+//
+// RUN: %clang -fsyntax-only -F %t -Xclang -code-completion-at=%s:7:24 %s -o - | FileCheck -check-prefix=CHECK-3 %s
+// CHECK-3: FOOInternal.h>