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

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

Differential Revision: https://reviews.llvm.org/D156704

clang-tools-extra/clang-include-fixer/IncludeFixer.cpp

@@ -314,11 +314,11 @@ std::string IncludeFixerSemaSource::minimizeInclude(
   if (!Entry)
     return std::string(Include);
 
-  bool IsSystem = false;
+  bool IsAngled = false;
   std::string Suggestion =
-      HeaderSearch.suggestPathToFileForDiagnostics(*Entry, "", &IsSystem);
+      HeaderSearch.suggestPathToFileForDiagnostics(*Entry, "", &IsAngled);
 
-  return IsSystem ? '<' + Suggestion + '>' : '"' + Suggestion + '"';
+  return IsAngled ? '<' + Suggestion + '>' : '"' + Suggestion + '"';
 }
 
 /// Get the include fixer context for the queried symbol.

clang-tools-extra/clangd/Headers.cpp

@@ -286,11 +286,11 @@ IncludeInserter::calculateIncludePath(const HeaderFile &InsertedHeader,
   assert(InsertedHeader.valid());
   if (InsertedHeader.Verbatim)
     return InsertedHeader.File;
-  bool IsSystem = false;
+  bool IsAngled = false;
   std::string Suggested;
   if (HeaderSearchInfo) {
     Suggested = HeaderSearchInfo->suggestPathToFileForDiagnostics(
-        InsertedHeader.File, BuildDir, IncludingFile, &IsSystem);
+        InsertedHeader.File, BuildDir, IncludingFile, &IsAngled);
   } else {
     // Calculate include relative to including file only.
     StringRef IncludingDir = llvm::sys::path::parent_path(IncludingFile);
@@ -303,7 +303,7 @@ IncludeInserter::calculateIncludePath(const HeaderFile &InsertedHeader,
   // FIXME: should we allow (some limited number of) "../header.h"?
   if (llvm::sys::path::is_absolute(Suggested))
     return std::nullopt;
-  if (IsSystem)
+  if (IsAngled)
     Suggested = "<" + Suggested + ">";
   else
     Suggested = "\"" + Suggested + "\"";

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

@@ -328,42 +328,33 @@ class SymbolCollector::HeaderFileURICache {
   // <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)
+      CachedSpelling->PublicHeader = llvm::formatv("<{0}/{0}.h>", Framework);
 
     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)
+      CachedSpelling->PrivateHeader =
+          llvm::formatv("<{0}/{0}_Private.h>", Framework);
+
     return HeaderPath.IsPrivateHeader ? CachedSpelling->PrivateHeader
                                       : CachedSpelling->PublicHeader;
   }
@@ -386,21 +377,14 @@ class SymbolCollector::HeaderFileURICache {
       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();
+    *CachedHeaderSpelling =
+        llvm::formatv("<{0}/{1}>", Framework, HeaderPath->HeaderSubpath).str();
     return llvm::StringRef(*CachedHeaderSpelling);
   }
 

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

@@ -702,9 +702,9 @@ TEST_F(SymbolCollectorTest, ObjCFrameworkIncludeHeader) {
   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.
@@ -722,13 +722,13 @@ TEST_F(SymbolCollectorTest, ObjCFrameworkIncludeHeader) {
                "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,
+      UnorderedElementsAre(
+          AllOf(qName("NSObject"), includeHeader("<Foundation/Foundation.h>")),
+          AllOf(qName("PrivateClass"),
+                includeHeader("<Foundation/Foundation_Private.h>")),
+          AllOf(qName("Container"))));
 
   runSymbolCollector(Header, Main,
                      {"-iframework", FrameworksPath, "-xobjective-c++"});

clang-tools-extra/include-cleaner/lib/HTMLReport.cpp

@@ -170,10 +170,10 @@ class Reporter {
   std::string spellHeader(const Header &H) {
     switch (H.kind()) {
     case Header::Physical: {
-      bool IsSystem = false;
+      bool IsAngled = false;
       std::string Path = HS.suggestPathToFileForDiagnostics(
-          H.physical(), MainFE->tryGetRealPathName(), &IsSystem);
-      return IsSystem ? "<" + Path + ">" : "\"" + Path + "\"";
+          H.physical(), MainFE->tryGetRealPathName(), &IsAngled);
+      return IsAngled ? "<" + Path + ">" : "\"" + Path + "\"";
     }
     case Header::Standard:
       return H.standard().name().str();

clang-tools-extra/include-cleaner/lib/IncludeSpeller.cpp

@@ -29,10 +29,10 @@ class DefaultIncludeSpeller : public IncludeSpeller {
     case Header::Verbatim:
       return Input.H.verbatim().str();
     case Header::Physical:
-      bool IsSystem = false;
+      bool IsAngled = false;
       std::string FinalSpelling = Input.HS.suggestPathToFileForDiagnostics(
-          Input.H.physical(), Input.Main->tryGetRealPathName(), &IsSystem);
-      return IsSystem ? "<" + FinalSpelling + ">" : "\"" + FinalSpelling + "\"";
+          Input.H.physical(), Input.Main->tryGetRealPathName(), &IsAngled);
+      return IsAngled ? "<" + FinalSpelling + ">" : "\"" + FinalSpelling + "\"";
     }
     llvm_unreachable("Unknown clang::include_cleaner::Header::Kind enum");
   }

clang/include/clang/Lex/HeaderSearch.h

@@ -871,11 +871,11 @@ class HeaderSearch {
   /// MainFile location, if none of the include search directories were prefix
   /// of File.
   ///
-  /// \param IsSystem If non-null, filled in to indicate whether the suggested
-  ///        path is relative to a system header directory.
+  /// \param IsAngled If non-null, filled in to indicate whether the suggested
+  ///        path should be referenced as <Header.h> instead of "Header.h".
   std::string suggestPathToFileForDiagnostics(const FileEntry *File,
                                               llvm::StringRef MainFile,
-                                              bool *IsSystem = nullptr) const;
+                                              bool *IsAngled = nullptr) const;
 
   /// Suggest a path by which the specified file could be found, for use in
   /// diagnostics to suggest a #include. Returned path will only contain forward
@@ -889,7 +889,7 @@ class HeaderSearch {
   std::string suggestPathToFileForDiagnostics(llvm::StringRef File,
                                               llvm::StringRef WorkingDir,
                                               llvm::StringRef MainFile,
-                                              bool *IsSystem = nullptr) const;
+                                              bool *IsAngled = nullptr) const;
 
   void PrintStats();
 

clang/lib/Lex/HeaderSearch.cpp

@@ -1928,17 +1928,17 @@ void HeaderSearch::loadSubdirectoryModuleMaps(DirectoryLookup &SearchDir) {
 }
 
 std::string HeaderSearch::suggestPathToFileForDiagnostics(
-    const FileEntry *File, llvm::StringRef MainFile, bool *IsSystem) const {
+    const FileEntry *File, llvm::StringRef MainFile, bool *IsAngled) 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);
+                                         MainFile, IsAngled);
 }
 
 std::string HeaderSearch::suggestPathToFileForDiagnostics(
     llvm::StringRef File, llvm::StringRef WorkingDir, llvm::StringRef MainFile,
-    bool *IsSystem) const {
+    bool *IsAngled) const {
   using namespace llvm::sys;
 
   llvm::SmallString<32> FilePath = File;
@@ -1996,15 +1996,16 @@ std::string HeaderSearch::suggestPathToFileForDiagnostics(
     if (DL.isNormalDir()) {
       StringRef Dir = DL.getDirRef()->getName();
       if (CheckDir(Dir)) {
-        if (IsSystem)
-          *IsSystem = BestPrefixLength && isSystem(DL.getDirCharacteristic());
+        if (IsAngled)
+          *IsAngled = BestPrefixLength && isSystem(DL.getDirCharacteristic());
         BestPrefixIsFramework = false;
       }
     } else if (DL.isFramework()) {
       StringRef Dir = DL.getFrameworkDirRef()->getName();
       if (CheckDir(Dir)) {
-        if (IsSystem)
-          *IsSystem = BestPrefixLength && isSystem(DL.getDirCharacteristic());
+        // Framework includes by convention use <>.
+        if (IsAngled)
+          *IsAngled = BestPrefixLength;
         BestPrefixIsFramework = true;
       }
     }
@@ -2013,8 +2014,8 @@ std::string HeaderSearch::suggestPathToFileForDiagnostics(
   // 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))) {
-    if (IsSystem)
-      *IsSystem = false;
+    if (IsAngled)
+      *IsAngled = false;
     BestPrefixIsFramework = false;
   }
 

clang/lib/Sema/SemaLookup.cpp

@@ -5701,10 +5701,10 @@ void Sema::diagnoseMissingImport(SourceLocation Loc, const NamedDecl *Decl,
 /// suggesting the addition of a #include of the specified file.
 static std::string getHeaderNameForHeader(Preprocessor &PP, const FileEntry *E,
                                           llvm::StringRef IncludingFile) {
-  bool IsSystem = false;
+  bool IsAngled = false;
   auto Path = PP.getHeaderSearchInfo().suggestPathToFileForDiagnostics(
-      E, IncludingFile, &IsSystem);
-  return (IsSystem ? '<' : '"') + Path + (IsSystem ? '>' : '"');
+      E, IncludingFile, &IsAngled);
+  return (IsAngled ? '<' : '"') + Path + (IsAngled ? '>' : '"');
 }
 
 void Sema::diagnoseMissingImport(SourceLocation UseLoc, const NamedDecl *Decl,

clang/unittests/Lex/HeaderSearchTest.cpp

@@ -47,14 +47,18 @@ class HeaderSearchTest : public ::testing::Test {
     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);
-    Search.AddSystemSearchPath(DL);
+    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,
@@ -175,20 +179,32 @@ TEST_F(HeaderSearchTest, IncludeFromSameDirectory) {
 }
 
 TEST_F(HeaderSearchTest, SdkFramework) {
-  addSystemFrameworkSearchDir(
+  addFrameworkSearchDir(
       "/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk/Frameworks/");
-  bool IsSystem = false;
+  bool IsAngled = false;
   EXPECT_EQ(Search.suggestPathToFileForDiagnostics(
                 "/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/"
                 "Frameworks/AppKit.framework/Headers/NSView.h",
                 /*WorkingDir=*/"",
-                /*MainFile=*/"", &IsSystem),
+                /*MainFile=*/"", &IsAngled),
             "AppKit/NSView.h");
-  EXPECT_TRUE(IsSystem);
+  EXPECT_TRUE(IsAngled);
+
+  addFrameworkSearchDir("/System/Developer/Library/Framworks/",
+                        /*IsSystem*/ false);
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics(
+                "/System/Developer/Library/Framworks/"
+                "Foo.framework/Headers/Foo.h",
+                /*WorkingDir=*/"",
+                /*MainFile=*/"", &IsAngled),
+            "Foo/Foo.h");
+  // Expect to be true even though we passed false to IsSystem earlier since
+  // all frameworks should be treated as <>.
+  EXPECT_TRUE(IsAngled);
 }
 
 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",
@@ -199,7 +215,7 @@ TEST_F(HeaderSearchTest, NestedFramework) {
 
 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,