[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/clang-include-fixer/IncludeFixer.cpp

std::string IncludeFixerSemaSource::minimizeInclude(
   StringRef StrippedInclude = Include.trim("\"<>");
   auto Entry = SourceManager.getFileManager().getFile(StrippedInclude);
 
   // If the file doesn't exist return the path from the database.
   // FIXME: This should never happen.
   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.
 IncludeFixerContext IncludeFixerSemaSource::getIncludeFixerContext(
     const clang::SourceManager &SourceManager,
     clang::HeaderSearch &HeaderSearch,
     ArrayRef<find_all_symbols::SymbolInfo> MatchedSymbols) const {
   std::vector<find_all_symbols::SymbolInfo> SymbolCandidates;

clang-tools-extra/clangd/Headers.cpp

 }
 
 std::optional<std::string>
 IncludeInserter::calculateIncludePath(const HeaderFile &InsertedHeader,
                                       llvm::StringRef IncludingFile) const {
   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);
     SmallString<256> RelFile(InsertedHeader.File);
     // Replacing with "" leaves "/RelFile" if IncludingDir doesn't end in "/".
     llvm::sys::path::replace_path_prefix(RelFile, IncludingDir, "./");
     Suggested = llvm::sys::path::convert_to_slash(
         llvm::sys::path::remove_leading_dotslash(RelFile));
   }
   // 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 + "\"";
   return Suggested;
 }
 
 std::optional<TextEdit>
 IncludeInserter::insert(llvm::StringRef VerbatimHeader,

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-tools-extra/include-cleaner/lib/HTMLReport.cpp

if (llvm::any_of(List, [&](unsigned I) {
         }))
       return "used";
     return "semiused";
   }
 
   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();
     case Header::Verbatim:
       return H.verbatim().str();
     }
     llvm_unreachable("Unknown Header kind");
   }

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

 public:
   std::string operator()(const Input &Input) const override {
     switch (Input.H.kind()) {
     case Header::Standard:
       return Input.H.standard().name().str();
     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");
   }
 };
 
 } // namespace
 
 std::string spellHeader(const IncludeSpeller::Input &Input) {
for (const auto &Speller : *Spellers) {
     Spelling = (*Speller)(Input);
     if (!Spelling.empty())
       break;
   }
   return Spelling;
 }
 
 } // namespace clang::include_cleaner
 No newline at end of file

clang/include/clang/Lex/HeaderSearch.h

public:
 
   /// 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
   /// slashes as separators. MainFile is the absolute path of the file that we
   /// are generating the diagnostics for. It will try to shorten the path using
   /// 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
   /// slashes as separators. MainFile is the absolute path of the file that we
   /// are generating the diagnostics for. It will try to shorten the path using
   /// MainFile location, if none of the include search directories were prefix
   /// of File.
   ///
   /// \param WorkingDir If non-empty, this will be prepended to search directory
   /// paths that are relative.
   std::string suggestPathToFileForDiagnostics(llvm::StringRef File,
                                               llvm::StringRef WorkingDir,
                                               llvm::StringRef MainFile,
-                                              bool *IsSystem = nullptr) const;
+                                              bool *IsAngled = nullptr) const;
 
   void PrintStats();
 
   size_t getTotalMemory() const;
 
 private:
   /// Describes what happened when we tried to load a module map file.
   enum LoadModuleMapResult {

clang/lib/Lex/HeaderSearch.cpp

if (IsFramework == SearchDir.isFramework())
       loadModuleMapFile(Dir->path(), SearchDir.isSystemHeaderDirectory(),
                         SearchDir.isFramework());
   }
 
   SearchDir.setSearchedAllModuleMaps(true);
 }
 
 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;
   // 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).
   path::remove_dots(FilePath, /*remove_dot_dot=*/true);
   path::native(FilePath, path::Style::posix);
auto CheckDir = [&](llvm::SmallString<32> Dir) -> bool {
     return false;
   };
 
   bool BestPrefixIsFramework = false;
   for (const DirectoryLookup &DL : search_dir_range()) {
     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;
       }
     }
   }
 
   // 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;
   }
 
   // Try resolving resulting filename via reverse search in header maps,
   // key from header name is user preferred name for the include file.
   StringRef Filename = File.drop_front(BestPrefixLength);
   for (const DirectoryLookup &DL : search_dir_range()) {
     if (!DL.isHeaderMap())

clang/lib/Sema/SemaLookup.cpp

void Sema::diagnoseMissingImport(SourceLocation Loc, const NamedDecl *Decl,
   diagnoseMissingImport(Loc, Def, Def->getLocation(), OwningModules, MIK,
                         Recover);
 }
 
 /// Get a "quoted.h" or <angled.h> include path to use in a diagnostic
 /// 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,
                                  SourceLocation DeclLoc,
                                  ArrayRef<Module *> Modules,
                                  MissingImportKind MIK, bool Recover) {
   assert(!Modules.empty());
 

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;
+  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",
                 /*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,