[clang][HeaderSearch] Treat framework headers as System for suggestPath

[dgoldman]

Summary

• Rename the IsSystem flag to be IsAngled since that's how callers actually use the flag.
• Since frameworks by convention use <> style includes, make sure we treat them as Angled
  
  Also update clangd's custom logic for frameworks accordingly.

[dgoldman]

run clang-format

[kadircet]

clang/lib/Lex/HeaderSearch.cpp

1939

sorry I guess my suggestion get distorted a little bit on the way :D

I was actually suggesting to rename IsSystem to IsAngled, and change the logic in the rest of the code here to look for angled-ness of include search path, rather than system-ness.
As all of the current callers make use of this information to figure out if they should spell the includes with angles or quotes and set it if the include search dir is either part of angled or system includes.

That way we make sure we're not giving mixed signals out of this function. WDYT?

[dgoldman]

Rename IsSystem to IsAngled

[dgoldman]

run clang-format again

[kadircet]

thanks!

clang-tools-extra/clangd/index/SymbolCollector.cpp

splitFrameworkHeaderPath(llvm::StringRef Path) {
     return std::nullopt;
   }
 
   // Frameworks typically have an umbrella header of the same name, e.g.
   // <Foundation/Foundation.h> instead of <Foundation/NSObject.h> or
   // <Foundation/Foundation_Private.h> instead of
   // <Foundation/NSObject_Private.h> which should be used instead of directly
   // importing the header.
-  std::optional<std::string> getFrameworkUmbrellaSpelling(
-      llvm::StringRef Framework, SrcMgr::CharacteristicKind HeadersDirKind,
-      const HeaderSearch &HS, FrameworkHeaderPath &HeaderPath) {
+  std::optional<std::string>
+  getFrameworkUmbrellaSpelling(llvm::StringRef Framework,
+                               const HeaderSearch &HS,
+                               FrameworkHeaderPath &HeaderPath) {
     auto Res = CacheFrameworkToUmbrellaHeaderSpelling.try_emplace(Framework);
     auto *CachedSpelling = &Res.first->second;
     if (!Res.second) {
       return HeaderPath.IsPrivateHeader ? CachedSpelling->PrivateHeader
                                         : CachedSpelling->PublicHeader;
     }
-    bool IsSystem = isSystem(HeadersDirKind);
     SmallString<256> UmbrellaPath(HeaderPath.HeadersParentDir);
     llvm::sys::path::append(UmbrellaPath, "Headers", Framework + ".h");
 
     llvm::vfs::Status Status;
     auto StatErr = HS.getFileMgr().getNoncachedStatValue(UmbrellaPath, Status);
-    if (!StatErr) {
-      if (IsSystem)
-        CachedSpelling->PublicHeader = llvm::formatv("<{0}/{0}.h>", Framework);
-      else
-        CachedSpelling->PublicHeader =
-            llvm::formatv("\"{0}/{0}.h\"", Framework);
-    }
+    if (!StatErr)
 
     UmbrellaPath = HeaderPath.HeadersParentDir;
     llvm::sys::path::append(UmbrellaPath, "PrivateHeaders",
                             Framework + "_Private.h");
 
     StatErr = HS.getFileMgr().getNoncachedStatValue(UmbrellaPath, Status);
-    if (!StatErr) {
-      if (IsSystem)
-        CachedSpelling->PrivateHeader =
-            llvm::formatv("<{0}/{0}_Private.h>", Framework);
-      else
-        CachedSpelling->PrivateHeader =
-            llvm::formatv("\"{0}/{0}_Private.h\"", Framework);
-    }
+    if (!StatErr)
+
     return HeaderPath.IsPrivateHeader ? CachedSpelling->PrivateHeader
                                       : CachedSpelling->PublicHeader;
   }
 
   // Compute the framework include spelling for `FE` which is in a framework
   // named `Framework`, e.g. `NSObject.h` in framework `Foundation` would
   // give <Foundation/Foundation.h> if the umbrella header exists, otherwise
   // <Foundation/NSObject.h>.
   std::optional<llvm::StringRef> getFrameworkHeaderIncludeSpelling(
       const FileEntry *FE, llvm::StringRef Framework, HeaderSearch &HS) {
     auto Res = CachePathToFrameworkSpelling.try_emplace(FE->getName());
     auto *CachedHeaderSpelling = &Res.first->second;
     if (!Res.second)
       return llvm::StringRef(*CachedHeaderSpelling);
 
     auto HeaderPath = splitFrameworkHeaderPath(FE->getName());
     if (!HeaderPath) {
       // Unexpected: must not be a proper framework header, don't cache the
       // failure.
       CachePathToFrameworkSpelling.erase(Res.first);
       return std::nullopt;
     }
-    auto DirKind = HS.getFileDirFlavor(FE);
     if (auto UmbrellaSpelling =
-            getFrameworkUmbrellaSpelling(Framework, DirKind, HS, *HeaderPath)) {
+            getFrameworkUmbrellaSpelling(Framework, HS, *HeaderPath)) {
       *CachedHeaderSpelling = *UmbrellaSpelling;
       return llvm::StringRef(*CachedHeaderSpelling);
     }
 
-    if (isSystem(DirKind))
-      *CachedHeaderSpelling =
-          llvm::formatv("<{0}/{1}>", Framework, HeaderPath->HeaderSubpath)
-              .str();
-    else
-      *CachedHeaderSpelling =
-          llvm::formatv("\"{0}/{1}\"", Framework, HeaderPath->HeaderSubpath)
-              .str();
+        llvm::formatv("<{0}/{1}>", Framework, HeaderPath->HeaderSubpath).str();
     return llvm::StringRef(*CachedHeaderSpelling);
   }
 
   llvm::StringRef getIncludeHeaderUncached(FileID FID) {
     const auto FE = SM.getFileEntryRefForID(FID);
     if (!FE || FE->getName().empty())
       return "";
 

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

std::string Header = R"(
     @end
   )";
   std::string Main = "";
   TestFileName = testPath("test.m");
   runSymbolCollector(Header, Main, {"-F", FrameworksPath, "-xobjective-c++"});
   EXPECT_THAT(
       Symbols,
       UnorderedElementsAre(
-          AllOf(qName("NSObject"), includeHeader("\"Foundation/NSObject.h\"")),
+          AllOf(qName("NSObject"), includeHeader("<Foundation/NSObject.h>")),
           AllOf(qName("PrivateClass"),
-                includeHeader("\"Foundation/NSObject+Private.h\"")),
+                includeHeader("<Foundation/NSObject+Private.h>")),
           AllOf(qName("Container"))));
 
   // After adding the umbrella headers, we should use that spelling instead.
   std::string UmbrellaHeader = R"(
     #import <Foundation/NSObject.h>
   )";
   InMemoryFileSystem->addFile(
       testPath("Frameworks/Foundation.framework/Headers/Foundation.h"), 0,
       llvm::MemoryBuffer::getMemBuffer(UmbrellaHeader));
   std::string PrivateUmbrellaHeader = R"(
     #import <Foundation/NSObject+Private.h>
   )";
   InMemoryFileSystem->addFile(
       testPath("Frameworks/Foundation.framework/PrivateHeaders/"
                "Foundation_Private.h"),
       0, llvm::MemoryBuffer::getMemBuffer(PrivateUmbrellaHeader));
   runSymbolCollector(Header, Main, {"-F", FrameworksPath, "-xobjective-c++"});
-  EXPECT_THAT(Symbols,
-              UnorderedElementsAre(
-                  AllOf(qName("NSObject"),
-                        includeHeader("\"Foundation/Foundation.h\"")),
-                  AllOf(qName("PrivateClass"),
-                        includeHeader("\"Foundation/Foundation_Private.h\"")),
-                  AllOf(qName("Container"))));
+  EXPECT_THAT(
+      Symbols,
+          AllOf(qName("NSObject"), includeHeader("<Foundation/Foundation.h>")),
+                includeHeader("<Foundation/Foundation_Private.h>")),
 
   runSymbolCollector(Header, Main,
                      {"-iframework", FrameworksPath, "-xobjective-c++"});
   EXPECT_THAT(
       Symbols,
       UnorderedElementsAre(
           AllOf(qName("NSObject"), includeHeader("<Foundation/Foundation.h>")),
           AllOf(qName("PrivateClass"),

clang/lib/Lex/HeaderSearch.cpp

std::string HeaderSearch::suggestPathToFileForDiagnostics(
     const FileEntry *File, llvm::StringRef MainFile, bool *IsSystem) const {
   // FIXME: We assume that the path name currently cached in the FileEntry is
   // the most appropriate one for this analysis (and that it's spelled the
   // same way as the corresponding header search path).
   return suggestPathToFileForDiagnostics(File->getName(), /*WorkingDir=*/"",
                                          MainFile, IsSystem);
 }
 
 std::string HeaderSearch::suggestPathToFileForDiagnostics(
     llvm::StringRef File, llvm::StringRef WorkingDir, llvm::StringRef MainFile,
     bool *IsSystem) const {
   using namespace llvm::sys;
 
   llvm::SmallString<32> FilePath = File;
   // remove_dots switches to backslashes on windows as a side-effect!
   // We always want to suggest forward slashes for includes.
   // (not remove_dots(..., posix) as that misparses windows paths).
if (DL.isNormalDir()) {
       if (CheckDir(Dir)) {
         if (IsSystem)
           *IsSystem = BestPrefixLength && isSystem(DL.getDirCharacteristic());
         BestPrefixIsFramework = false;
       }
     } else if (DL.isFramework()) {
       StringRef Dir = DL.getFrameworkDirRef()->getName();
       if (CheckDir(Dir)) {
+        // Framework includes by convention use <> so mark these as system
+        // so the caller know to use <> instead of "" as well.
         if (IsSystem)
-          *IsSystem = BestPrefixLength && isSystem(DL.getDirCharacteristic());
+          *IsSystem = BestPrefixLength;
         BestPrefixIsFramework = true;
       }
     }
   }
 
   // Try to shorten include path using TUs directory, if we couldn't find any
   // suitable prefix in include search paths.
   if (!BestPrefixLength && CheckDir(path::parent_path(MainFile))) {

clang/unittests/Lex/HeaderSearchTest.cpp

VFS->addFile(
         Dir, 0, llvm::MemoryBuffer::getMemBuffer(""), /*User=*/std::nullopt,
         /*Group=*/std::nullopt, llvm::sys::fs::file_type::directory_file);
     auto DE = FileMgr.getOptionalDirectoryRef(Dir);
     assert(DE);
     auto DL = DirectoryLookup(*DE, SrcMgr::C_User, /*isFramework=*/false);
     Search.AddSearchPath(DL, /*isAngled=*/false);
   }
 
-  void addSystemFrameworkSearchDir(llvm::StringRef Dir) {
+  void addFrameworkSearchDir(llvm::StringRef Dir, bool IsSystem = true) {
     VFS->addFile(
         Dir, 0, llvm::MemoryBuffer::getMemBuffer(""), /*User=*/std::nullopt,
         /*Group=*/std::nullopt, llvm::sys::fs::file_type::directory_file);
     auto DE = FileMgr.getOptionalDirectoryRef(Dir);
     assert(DE);
-    auto DL = DirectoryLookup(*DE, SrcMgr::C_System, /*isFramework=*/true);
+    auto DL = DirectoryLookup(*DE, IsSystem ? SrcMgr::C_System : SrcMgr::C_User,
+                              /*isFramework=*/true);
+    if (IsSystem)
       Search.AddSystemSearchPath(DL);
+    else
+      Search.AddSearchPath(DL, /*isAngled=*/true);
   }
 
   void addHeaderMap(llvm::StringRef Filename,
                     std::unique_ptr<llvm::MemoryBuffer> Buf,
                     bool isAngled = false) {
     VFS->addFile(Filename, 0, std::move(Buf), /*User=*/std::nullopt,
                  /*Group=*/std::nullopt,
                  llvm::sys::fs::file_type::regular_file);
TEST_F(HeaderSearchTest, IncludeFromSameDirectory) {
   addSearchDir("/");
   EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/y/z/t.h",
                                                    /*WorkingDir=*/"",
                                                    /*MainFile=*/"/y/a.cc"),
             "y/z/t.h");
 }
 
 TEST_F(HeaderSearchTest, SdkFramework) {
-  addSystemFrameworkSearchDir(
+  addFrameworkSearchDir(
       "/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk/Frameworks/");
   bool IsSystem = false;
   EXPECT_EQ(Search.suggestPathToFileForDiagnostics(
                 "/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/"
                 "Frameworks/AppKit.framework/Headers/NSView.h",
                 /*WorkingDir=*/"",
                 /*MainFile=*/"", &IsSystem),
             "AppKit/NSView.h");
   EXPECT_TRUE(IsSystem);
+
+  addFrameworkSearchDir("/System/Developer/Library/Framworks/",
+                        /*IsSystem*/ false);
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics(
+                "/System/Developer/Library/Framworks/"
+                "Foo.framework/Headers/Foo.h",
+                /*WorkingDir=*/"",
+                /*MainFile=*/"", &IsSystem),
+            "Foo/Foo.h");
+  // Expect to be true even though we passed false to IsSystem earlier since
+  // all frameworks should be treated as <>.
+  EXPECT_TRUE(IsSystem);
 }
 
 TEST_F(HeaderSearchTest, NestedFramework) {
-  addSystemFrameworkSearchDir("/Platforms/MacOSX/Frameworks");
+  addFrameworkSearchDir("/Platforms/MacOSX/Frameworks");
   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");
+  addFrameworkSearchDir("/tmp/Frameworks");
   VFS->addFile(HeaderPath, 0,
                llvm::MemoryBuffer::getMemBufferCopy("", HeaderPath),
                /*User=*/std::nullopt, /*Group=*/std::nullopt,
                llvm::sys::fs::file_type::regular_file);
 
   bool IsFrameworkFound = false;
   auto FoundFile = Search.LookupFile(
       "Foo/Foo.h", SourceLocation(), /*isAngled=*/true, /*FromDir=*/nullptr,