[clang][HeaderSearch] Support framework includes in suggestPath...

Clang will now search through the framework includes to identify the framework include path to a file, and then suggest a framework style include spelling for the file. Differential Revision: https://reviews.llvm.org/D115183
llvm · Jan 10, 2022 · 0cf7e61 · 0cf7e61
1 parent 7485e6c
commit 0cf7e61
Show file tree

Hide file tree

Showing 4 changed files with 118 additions and 26 deletions.
diff --git a/clang/include/clang/Lex/HeaderSearch.h b/clang/include/clang/Lex/HeaderSearch.h
@@ -286,6 +286,12 @@ class HeaderSearch {
   /// Add an additional search path.
   void AddSearchPath(const DirectoryLookup &dir, bool isAngled);
 
+  /// Add an additional system search path.
+  void AddSystemSearchPath(const DirectoryLookup &dir) {
+    SearchDirs.push_back(dir);
+    SearchDirsUsage.push_back(false);
+  }
+
   /// Set the list of system header prefixes.
   void SetSystemHeaderPrefixes(ArrayRef<std::pair<std::string, bool>> P) {
     SystemHeaderPrefixes.assign(P.begin(), P.end());

diff --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp
@@ -745,7 +745,8 @@ static const char *copyString(StringRef Str, llvm::BumpPtrAllocator &Alloc) {
 }
 
 static bool isFrameworkStylePath(StringRef Path, bool &IsPrivateHeader,
-                                 SmallVectorImpl<char> &FrameworkName) {
+                                 SmallVectorImpl<char> &FrameworkName,
+                                 SmallVectorImpl<char> &IncludeSpelling) {
   using namespace llvm::sys;
   path::const_iterator I = path::begin(Path);
   path::const_iterator E = path::end(Path);
@@ -761,15 +762,22 @@ static bool isFrameworkStylePath(StringRef Path, bool &IsPrivateHeader,
   // and some other variations among these lines.
   int FoundComp = 0;
   while (I != E) {
-    if (*I == "Headers")
+    if (*I == "Headers") {
       ++FoundComp;
-    if (I->endswith(".framework")) {
-      FrameworkName.append(I->begin(), I->end());
-      ++FoundComp;
-    }
-    if (*I == "PrivateHeaders") {
+    } else if (*I == "PrivateHeaders") {
       ++FoundComp;
       IsPrivateHeader = true;
+    } else if (I->endswith(".framework")) {
+      StringRef Name = I->drop_back(10); // Drop .framework
+      // Need to reset the strings and counter to support nested frameworks.
+      FrameworkName.clear();
+      FrameworkName.append(Name.begin(), Name.end());
+      IncludeSpelling.clear();
+      IncludeSpelling.append(Name.begin(), Name.end());
+      FoundComp = 1;
+    } else if (FoundComp >= 2) {
+      IncludeSpelling.push_back('/');
+      IncludeSpelling.append(I->begin(), I->end());
     }
     ++I;
   }
@@ -784,20 +792,24 @@ diagnoseFrameworkInclude(DiagnosticsEngine &Diags, SourceLocation IncludeLoc,
                          bool FoundByHeaderMap = false) {
   bool IsIncluderPrivateHeader = false;
   SmallString<128> FromFramework, ToFramework;
-  if (!isFrameworkStylePath(Includer, IsIncluderPrivateHeader, FromFramework))
+  SmallString<128> FromIncludeSpelling, ToIncludeSpelling;
+  if (!isFrameworkStylePath(Includer, IsIncluderPrivateHeader, FromFramework,
+                            FromIncludeSpelling))
     return;
   bool IsIncludeePrivateHeader = false;
-  bool IsIncludeeInFramework = isFrameworkStylePath(
-      IncludeFE->getName(), IsIncludeePrivateHeader, ToFramework);
+  bool IsIncludeeInFramework =
+      isFrameworkStylePath(IncludeFE->getName(), IsIncludeePrivateHeader,
+                           ToFramework, ToIncludeSpelling);
 
   if (!isAngled && !FoundByHeaderMap) {
     SmallString<128> NewInclude("<");
     if (IsIncludeeInFramework) {
-      NewInclude += ToFramework.str().drop_back(10); // drop .framework
-      NewInclude += "/";
+      NewInclude += ToIncludeSpelling;
+      NewInclude += ">";
+    } else {
+      NewInclude += IncludeFilename;
+      NewInclude += ">";
     }
-    NewInclude += IncludeFilename;
-    NewInclude += ">";
     Diags.Report(IncludeLoc, diag::warn_quoted_include_in_framework_header)
         << IncludeFilename
         << FixItHint::CreateReplacement(IncludeLoc, NewInclude);
@@ -1865,9 +1877,9 @@ std::string HeaderSearch::suggestPathToFileForDiagnostics(
   using namespace llvm::sys;
 
   unsigned BestPrefixLength = 0;
-  // Checks whether Dir and File shares a common prefix, if they do and that's
-  // the longest prefix we've seen so for it returns true and updates the
-  // BestPrefixLength accordingly.
+  // Checks whether `Dir` is a strict path prefix of `File`. If so and that's
+  // the longest prefix we've seen so for it, returns true and updates the
+  // `BestPrefixLength` accordingly.
   auto CheckDir = [&](llvm::StringRef Dir) -> bool {
     llvm::SmallString<32> DirPath(Dir.begin(), Dir.end());
     if (!WorkingDir.empty() && !path::is_absolute(Dir))
@@ -1902,26 +1914,48 @@ std::string HeaderSearch::suggestPathToFileForDiagnostics(
           path::is_separator(NI->front()) && path::is_separator(DI->front()))
         continue;
 
+      // Special case Apple .sdk folders since the search path is typically a
+      // symlink like `iPhoneSimulator14.5.sdk` while the file is instead
+      // located in `iPhoneSimulator.sdk` (the real folder).
+      if (NI->endswith(".sdk") && DI->endswith(".sdk")) {
+        StringRef NBasename = path::stem(*NI);
+        StringRef DBasename = path::stem(*DI);
+        if (DBasename.startswith(NBasename))
+          continue;
+      }
+
       if (*NI != *DI)
         break;
     }
     return false;
   };
 
+  bool BestPrefixIsFramework = false;
   for (unsigned I = 0; I != SearchDirs.size(); ++I) {
-    // FIXME: Support this search within frameworks.
-    if (!SearchDirs[I].isNormalDir())
-      continue;
-
-    StringRef Dir = SearchDirs[I].getDir()->getName();
-    if (CheckDir(Dir) && IsSystem)
-      *IsSystem = BestPrefixLength ? I >= SystemDirIdx : false;
+    if (SearchDirs[I].isNormalDir()) {
+      StringRef Dir = SearchDirs[I].getDir()->getName();
+      if (CheckDir(Dir)) {
+        if (IsSystem)
+          *IsSystem = BestPrefixLength ? I >= SystemDirIdx : false;
+        BestPrefixIsFramework = false;
+      }
+    } else if (SearchDirs[I].isFramework()) {
+      StringRef Dir = SearchDirs[I].getFrameworkDir()->getName();
+      if (CheckDir(Dir)) {
+        if (IsSystem)
+          *IsSystem = BestPrefixLength ? I >= SystemDirIdx : false;
+        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)) && IsSystem)
-    *IsSystem = false;
+  if (!BestPrefixLength && CheckDir(path::parent_path(MainFile))) {
+    if (IsSystem)
+      *IsSystem = false;
+    BestPrefixIsFramework = false;
+  }
 
   // Try resolving resulting filename via reverse search in header maps,
   // key from header name is user prefered name for the include file.
@@ -1934,8 +1968,19 @@ std::string HeaderSearch::suggestPathToFileForDiagnostics(
         SearchDirs[I].getHeaderMap()->reverseLookupFilename(Filename);
     if (!SpelledFilename.empty()) {
       Filename = SpelledFilename;
+      BestPrefixIsFramework = false;
       break;
     }
   }
+
+  // If the best prefix is a framework path, we need to compute the proper
+  // include spelling for the framework header.
+  bool IsPrivateHeader;
+  SmallString<128> FrameworkName, IncludeSpelling;
+  if (BestPrefixIsFramework &&
+      isFrameworkStylePath(Filename, IsPrivateHeader, FrameworkName,
+                           IncludeSpelling)) {
+    Filename = IncludeSpelling;
+  }
   return path::convert_to_slash(Filename);
 }
diff --git a/clang/test/Modules/double-quotes.m b/clang/test/Modules/double-quotes.m
@@ -24,8 +24,17 @@
 // because they only show up under the module A building context.
 // RUN: FileCheck --input-file=%t/stderr %s
 // CHECK: double-quoted include "A0.h" in framework header, expected angle-bracketed instead
+// CHECK: #include "A0.h"
+// CHECK:          ^~~~~~
+// CHECK: <A/A0.h>
 // CHECK: double-quoted include "B.h" in framework header, expected angle-bracketed instead
+// CHECK: #include "B.h"
+// CHECK:          ^~~~~
+// CHECK: <B.h>
 // CHECK: double-quoted include "B.h" in framework header, expected angle-bracketed instead
+// CHECK: #import "B.h" // Included from Z.h & A.h
+// CHECK:         ^~~~~
+// CHECK: <B.h>
 
 #import "A.h"
 #import <Z/Z.h>

diff --git a/clang/unittests/Lex/HeaderSearchTest.cpp b/clang/unittests/Lex/HeaderSearchTest.cpp
@@ -47,6 +47,15 @@ class HeaderSearchTest : public ::testing::Test {
     Search.AddSearchPath(DL, /*isAngled=*/false);
   }
 
+  void addSystemFrameworkSearchDir(llvm::StringRef Dir) {
+    VFS->addFile(Dir, 0, llvm::MemoryBuffer::getMemBuffer(""), /*User=*/None,
+                 /*Group=*/None, 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);
+  }
+
   void addHeaderMap(llvm::StringRef Filename,
                     std::unique_ptr<llvm::MemoryBuffer> Buf,
                     bool isAngled = false) {
@@ -155,6 +164,29 @@ TEST_F(HeaderSearchTest, IncludeFromSameDirectory) {
             "y/z/t.h");
 }
 
+TEST_F(HeaderSearchTest, SdkFramework) {
+  addSystemFrameworkSearchDir(
+      "/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);
+}
+
+TEST_F(HeaderSearchTest, NestedFramework) {
+  addSystemFrameworkSearchDir("/Platforms/MacOSX/Frameworks");
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics(
+                "/Platforms/MacOSX/Frameworks/AppKit.framework/Frameworks/"
+                "Sub.framework/Headers/Sub.h",
+                /*WorkingDir=*/"",
+                /*MainFile=*/""),
+            "Sub/Sub.h");
+}
+
 // Helper struct with null terminator character to make MemoryBuffer happy.
 template <class FileTy, class PaddingTy>
 struct NullTerminatedFile : public FileTy {