[HeaderSearch] Track framework name in LookupFile

[dgoldman]

Summary

Previously, the Framework name was only set if the file
came from a header mapped framework; now we'll always
set the framework name if the file is in a framework.

[sammccall]

Thanks for splitting this out from the other patch. This feels subtle and I don't think I know enough about frameworks to review.

@cishida made a recent change here which @jansvoboda11 reviewed - I'm guessing they'd know what to do here.

clang/lib/Lex/HeaderSearch.cpp

1039–1040

The comment here explicitly say "in a header map and". Unfortunately it doesn't say why, and I don't know enough to guess.

[dgoldman]

Cyndy and Jan, hopefully ya'll are the right reviewers here? We're using this Framework information to know to compute framework style includes in https://reviews.llvm.org/D117056 for clangd, please LMK if this is a bad idea and we should do something else.

[cishida]

My understanding for the header map restriction is because headermaps are generally emitted once for a framework build and only consumed to build such framework and this struct information is only queried during this.
It sounds like for the clangd support, you're not as interested in what framework is being built, but the name of the framework when included via framework style in any context. Is that correct? It seems fine to extend its use cases. @jansvoboda11 What do you think?

[dgoldman]

In D117830#3267404, @cishida wrote:

My understanding for the header map restriction is because headermaps are generally emitted once for a framework build and only consumed to build such framework and this struct information is only queried during this.
It sounds like for the clangd support, you're not as interested in what framework is being built, but the name of the framework when included via framework style in any context. Is that correct? It seems fine to extend its use cases. @jansvoboda11 What do you think?

Yep, exactly, for clangd, we want to know when a header is included via a framework style include (although maybe it would be better to know if a header is actually apart of a framework regardless of the spelling to import it?) so we can generate a proper framework style include for symbols in the framework. That lets us suggest an include of <UIKit/UIKit.h> for UIView, for example.

[cishida]

I followed up about this approach with folks more familiar with SourceKit and seems fine, so LGTM. I'd wait a day to see if Jan has any concerns.

clang/lib/Lex/HeaderSearch.cpp

1039–1040

nit/reminder: This should be updated now

[jansvoboda11]

I don't have any suggestions here except for running the patch through clang-format.

[dgoldman]

Update comment + run clang-format

[dgoldman]

clang/lib/Lex/HeaderSearch.cpp

1039–1040

Updated, LMK if you have any suggestions on the wording here.

[cishida]

clang/lib/Lex/HeaderSearch.cpp

1039–1040

LGTM

clang/lib/Lex/HeaderSearch.cpp

for (; i != SearchDirs.size(); ++i) {
     // whether the file is a system header.
     for (unsigned j = SystemHeaderPrefixes.size(); j; --j) {
       if (Filename.startswith(SystemHeaderPrefixes[j-1].first)) {
         HFI.DirInfo = SystemHeaderPrefixes[j-1].second ? SrcMgr::C_System
                                                        : SrcMgr::C_User;
         break;
       }
     }
 
-    // If this file is found in a header map and uses the framework style of
-    // includes, then this header is part of a framework we're building.
+    // Set the `Framework` info if this file is in a header map with framework
+    // style include spelling or found in a framework dir. The header map case
+    // is possible when building frameworks which use header maps.
     if (CurDir->isHeaderMap() && isAngled) {
       size_t SlashPos = Filename.find('/');
       if (SlashPos != StringRef::npos)
         HFI.Framework =
             getUniqueFrameworkName(StringRef(Filename.begin(), SlashPos));
       if (CurDir->isIndexHeaderMap())
         HFI.IndexHeaderMapHeader = 1;
+    } else if (CurDir->isFramework()) {
+      size_t SlashPos = Filename.find('/');
+      if (SlashPos != StringRef::npos)
+        HFI.Framework =
+            getUniqueFrameworkName(StringRef(Filename.begin(), SlashPos));
     }
 
     if (checkMSVCHeaderSearch(Diags, MSFE ? &MSFE->getFileEntry() : nullptr,
                               &File->getFileEntry(), IncludeLoc)) {
       if (SuggestedModule)
         *SuggestedModule = MSSuggestedModule;
       return MSFE;
     }

clang/unittests/Lex/HeaderSearchTest.cpp

TEST_F(HeaderSearchTest, NestedFramework) {
   EXPECT_EQ(Search.suggestPathToFileForDiagnostics(
                 "/Platforms/MacOSX/Frameworks/AppKit.framework/Frameworks/"
                 "Sub.framework/Headers/Sub.h",
                 /*WorkingDir=*/"",
                 /*MainFile=*/""),
             "Sub/Sub.h");
 }
 
+TEST_F(HeaderSearchTest, HeaderFrameworkLookup) {
+  std::string HeaderPath = "/tmp/Frameworks/Foo.framework/Headers/Foo.h";
+  addSystemFrameworkSearchDir("/tmp/Frameworks");
+  VFS->addFile(
+      HeaderPath, 0, llvm::MemoryBuffer::getMemBufferCopy("", HeaderPath),
+      /*User=*/None, /*Group=*/None, llvm::sys::fs::file_type::regular_file);
+
+  bool IsFrameworkFound = false;
+  auto FoundFile = Search.LookupFile(
+      "Foo/Foo.h", SourceLocation(), /*isAngled=*/true, /*FromDir=*/nullptr,
+      /*CurDir=*/nullptr, /*Includers=*/{}, /*SearchPath=*/nullptr,
+      /*RelativePath=*/nullptr, /*RequestingModule=*/nullptr,
+      /*SuggestedModule=*/nullptr, /*IsMapped=*/nullptr, &IsFrameworkFound);
+
+  EXPECT_TRUE(FoundFile.hasValue());
+  EXPECT_TRUE(IsFrameworkFound);
+  auto &FE = FoundFile.getValue();
+  auto FI = Search.getExistingFileInfo(FE);
+  EXPECT_TRUE(FI);
+  EXPECT_TRUE(FI->IsValid);
+  EXPECT_EQ(FI->Framework.str(), "Foo");
+}
+
 // Helper struct with null terminator character to make MemoryBuffer happy.
 template <class FileTy, class PaddingTy>
 struct NullTerminatedFile : public FileTy {
   PaddingTy Padding = 0;
 };
 
 TEST_F(HeaderSearchTest, HeaderMapReverseLookup) {
   typedef NullTerminatedFile<test::HMapFileMock<2, 32>, char> FileTy;