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

[dgoldman]

Summary

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.

[dgoldman]

Fix IncludeSpelling issue

[dgoldman]

Use consistent IsSystem detection

[dgoldman]

Add HeaderSearch tests

[sammccall]

This looks pretty nice to me, but I'm not convinced it's a good idea to include the umbrella header heuristic.

clang/lib/Lex/HeaderSearch.cpp

807–811

Nice!

No good deed goes un-punished: it should be possible to add a diagnostic test?
There are some under clang/test/Preprocessor/*framework*

1887

Seems like it'd be more consistent with the existing IsSystem to expand the callsites as

if (... && CheckDir(...)) {
  if (IsSystem)
    *IsSystem = whatever;
  IsFramework = whatever;
}

instead of taking a param here. (And honestly the CheckDir() && IsSystem thing is confusing, so it'd help)

1919

Nice comment, very helpful!

Just wanted to check you're sure the versioned one is the symlink and the unversioned one is the real folder, rather than the other way. (Plausible, just surprising to me)

1937

nit: /*IsFramework=*/, one star no spaces

1990

This heuristic looks iffy to me, it's not just trying to help the caller insert the right header, but tell them they want a different one for style reasons.

The first example framework in https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPFrameworks/Concepts/FrameworkAnatomy.html doesn't include an umbrella header of this form.

Moreover, non-framework libraries have umbrella headers too, this seems like an orthogonal concern best caught in some other way.

[dgoldman]

Fixes for review - update test

[dgoldman]

clang/lib/Lex/HeaderSearch.cpp

807–811

I couldn't seem to get it to generate a warning for a subframework, for both there and in double-quotes.m under Modules, so instead I added a simple test to headersearch for the sub framework case and made sure the fix it in double-quotes.m was OK for the existing cases

1919

Yep,

$ readlink /Applications/Xcode_13.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.0.sdk
MacOSX.sdk

1990

See https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPFrameworks/Tasks/IncludingFrameworks.html, it's common to use this umbrella header (especially for Apple SDKs) and some frameworks require it (as opposed to including the header directly), thus I think this default makes sense even if it isn't 100%.

Any other ideas on how we can help here? I guess if we had FS access we could see if the umbrella header exists...

[sammccall]

Thanks, test seems sensible.
I remain unconvinced that the umbrella stuff belongs here.
Maybe pull it out of this patch and send it separately if you want to discuss further, or get more opinions?

clang/lib/Lex/HeaderSearch.cpp

1990

I get that it's often a good idea/mandatory to include the umbrella header (for many libraries, not just frameworks), but it's not this function's job to choose which file to include, it's to find the best spelling of the file that was requested.

Similarly it doesn't check if the filename is "bits/shared_ptr.h" and suggest <shared_ptr> instead.

Any other ideas on how we can help here?

It depends what you're trying to achieve, maybe write a clang-tidy check instead? Or if this is something the compiler itself needs to care about, write a separate function to do the heuristic correction.
If you need the spelled, heuristically-corrected name, you can correct it first and then call this code, rather than having the spelling code do heuristic correction.

clang/test/Modules/double-quotes.m

27

it's hard to understand the context of how these headers are named, is there any other text on the line you can include in the CHECK?

[dgoldman]

Improve double-quotes.m test

[dgoldman]

Don't suggest umbrella headers

[sammccall]

clang/lib/Lex/HeaderSearch.cpp

1979

nit: I don't think these strings need to be so big, framework includes are a fairly uncommon case probably?

[sammccall]

clang/lib/Lex/HeaderSearch.cpp

1979

nvm, not sure where I got that idea.

clang/include/clang/Lex/HeaderSearch.h

public:
   /// Interface for setting the file search paths.
   void SetSearchPaths(std::vector<DirectoryLookup> dirs, unsigned angledDirIdx,
                       unsigned systemDirIdx, bool noCurDirSearch,
                       llvm::DenseMap<unsigned, unsigned> searchDirToHSEntry);
 
   /// 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());
   }
 
   /// Checks whether the map exists or not.
   bool HasIncludeAliasMap() const { return (bool)IncludeAliases; }
 

clang/lib/Lex/HeaderSearch.cpp

static const char *copyString(StringRef Str, llvm::BumpPtrAllocator &Alloc) {
   assert(!Str.empty());
   char *CopyStr = Alloc.Allocate<char>(Str.size()+1);
   std::copy(Str.begin(), Str.end(), CopyStr);
   CopyStr[Str.size()] = '\0';
   return CopyStr;
 }
 
 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);
   IsPrivateHeader = false;
 
   // Detect different types of framework style paths:
   //
   //   ...Foo.framework/{Headers,PrivateHeaders}
   //   ...Foo.framework/Versions/{A,Current}/{Headers,PrivateHeaders}
   //   ...Foo.framework/Frameworks/Nested.framework/{Headers,PrivateHeaders}
   //   ...<other variations with 'Versions' like in the above path>
   //
   // 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;
   }
 
   return !FrameworkName.empty() && FoundComp >= 2;
 }
 
 static void
 diagnoseFrameworkInclude(DiagnosticsEngine &Diags, SourceLocation IncludeLoc,
                          StringRef Includer, StringRef IncludeFilename,
                          const FileEntry *IncludeFE, bool isAngled = false,
                          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 += ">";
+    }
     Diags.Report(IncludeLoc, diag::warn_quoted_include_in_framework_header)
         << IncludeFilename
         << FixItHint::CreateReplacement(IncludeLoc, NewInclude);
   }
 
   // Headers in Foo.framework/Headers should not include headers
   // from Foo.framework/PrivateHeaders, since this violates public/private
   // API boundaries and can cause modular dependency cycles.

 }
 
 std::string HeaderSearch::suggestPathToFileForDiagnostics(
     llvm::StringRef File, llvm::StringRef WorkingDir, llvm::StringRef MainFile,
     bool *IsSystem) {
   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))
       fs::make_absolute(WorkingDir, DirPath);
     path::remove_dots(DirPath, /*remove_dot_dot=*/true);
     Dir = DirPath;
     for (auto NI = path::begin(File), NE = path::end(File),
               DI = path::begin(Dir), DE = path::end(Dir);
          /*termination condition in loop*/; ++NI, ++DI) {
       // '.' components in File are ignored.
       while (NI != NE && *NI == ".")
         ++NI;
       if (NI == NE)
for (auto NI = path::begin(File), NE = path::end(File),
         break;
       }
 
       // Consider all path separators equal.
       if (NI->size() == 1 && DI->size() == 1 &&
           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;
-
+    if (SearchDirs[I].isNormalDir()) {
       StringRef Dir = SearchDirs[I].getDir()->getName();
-    if (CheckDir(Dir) && IsSystem)
+      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)
+  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.
   StringRef Filename = File.drop_front(BestPrefixLength);
   for (unsigned I = 0; I != SearchDirs.size(); ++I) {
     if (!SearchDirs[I].isHeaderMap())
       continue;
 
     StringRef SpelledFilename =
         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);
 }

clang/test/Modules/double-quotes.m

 // RUN:   -F%S/Inputs/double-quotes -I%S/Inputs/double-quotes \
 // RUN:   -Wquoted-include-in-framework-header -fsyntax-only %s \
 // RUN:   2>%t/stderr
 
 // The same warnings show up when modules is on but -verify doesn't get it
 // 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>
 
 // Make sure we correctly handle paths that resemble frameworks, but aren't.
 #import "NotAFramework/Headers/Headers/Thing1.h"
 
 int bar() { return foo(); }
 
 // expected-warning@Inputs/double-quotes/A.framework/Headers/A.h:1{{double-quoted include "A0.h" in framework header, expected angle-bracketed instead}}
 // expected-warning@Inputs/double-quotes/A.framework/Headers/A.h:2{{double-quoted include "B.h" in framework header, expected angle-bracketed instead}}
 // expected-warning@Inputs/double-quotes/flat-header-path/Z.h:1{{double-quoted include "B.h" in framework header, expected angle-bracketed instead}}

clang/unittests/Lex/HeaderSearchTest.cpp

void addSearchDir(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_User, /*isFramework=*/false);
     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) {
     VFS->addFile(Filename, 0, std::move(Buf), /*User=*/None, /*Group=*/None,
                  llvm::sys::fs::file_type::regular_file);
     auto FE = FileMgr.getFile(Filename, true);
     assert(FE);
 
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(
+      "/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 {
   PaddingTy Padding = 0;
 };
 
 TEST_F(HeaderSearchTest, HeaderMapReverseLookup) {
   typedef NullTerminatedFile<test::HMapFileMock<2, 32>, char> FileTy;