[clangd] Properly compute framework-style include spelling

[dgoldman]

Summary

With this change, clangd now computes framework-style includes
for framework headers at indexing time.

This also updates HeaderSearch to consistently set the Framework
HeaderFileInfo field for framework headers.

[dgoldman]

Run clang-format

[sammccall]

Nice!

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

204

...ToUmbrellaHeaderSpelling?

(Unclear to me what values "umbrella include" would take)

267

return Optional<FrameworkInfo> instead of sentinel values

270

This name is very vague: splitFrameworkHeaderPath?
Similarly FrameworkInfo -> FrameworkHeaderPath.

(I was confused why this wasn't cached until I understood this *isn't* per-framework information, it's per-header information)

292

Short doc comment here. In particular what Framework contains

292

no need to pass Path, it's FE->getName()

298

there are 7 references to SpellingR.first->second in this function, give it a name?

300

Having multiple caches managed by this function makes it hard to understand.
Pull out a function to get the (cached) umbrella header for a framework?

302

Why? Seems like just looking up the umbrella first would be simpler and just as efficient.

310

if the header we happened to see first is PrivateHeaders, then you're caching the fact that this framework has no umbrella header.

There's a design decision here: is the path of any single header sufficient to decide whether a framework has an umbrella? If not, the cache needs to work differently.

315

comment here on what an umbrella header is and why we care

395

This comment is most useful to people not very familiar with frameworks, but isn't specific enough. maybe:
Framework headers are spelled as <Framework/Foo.h>, not "path/Framework.framework/Headers/Foo.h"

The bit about umbrella headers should go where we do that, and mostly say *why* rather than what

400

nit: inline, this var doesn't improve readability

402

return Optional<StringRef> and initialize this in the if, rather than using the empty string as a sentinel value

404

nit: no braces

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

669

nit: inline these filenames used once

671

nit: no const on locals

695

Instead of copying the setup 3 times, can we:

• run the test, expect "Foundation/NSObject.h"
• add the umbrella file
• run the test, expect "Foundation/Foundation.h"
• run the test with -isysroot, expect <Foundation/Foundation.h>

?

762

OOC, is "withsysroot" needed here?

988

unrelated whitespace changes

clang/lib/Lex/HeaderSearch.cpp

1052 ↗

(On Diff #399336)

This looks like a separate change, and should have a test in clang.

[dgoldman]

Changes for review

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

302

To give a proper spelling for private framework headers (although maybe the framework import would be more useful than a non-framework import? I dunno)

310

Fixed

315

Added a comment to the top of getFrameworkUmbrellaSpelling

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

762

Changed, should have been iframework to add a system framework path

[dgoldman]

clang/lib/Lex/HeaderSearch.cpp

1052 ↗

(On Diff #399336)

Will try to split out + add a test tomorrow

[sammccall]

This looks good now, only blocker is dropping the change to LookupFile somehow.

Is it valuable to land this before the imminent 14 branch cut? I think it's OK but as an indexing change a crash has the possibility to be really disruptive, and most of the rest of us aren't regularly dogfooding this on Mac.
I'd suggest we only land it before the branch if you feel like you can test this on a reasonable variety of code in the next few weeks. WDYT?

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

300

I'd suggest only passing the CharacteristicKind HeadersDir here rather than the whole FileEntry + HeaderPath.
Or failing that, being very explicit that these are just for an *example* header.

(The confusing part/trap here is that we're computing something based on one input, and then caching it so that it will be used for other inputs, without verifying that they would produce the same answer)

316

nit: this is a no-op

333

any reason not to also try_emplace here and reuse the iterator below?

341

We're (deliberately?) bypassing the umbrella cache here, say why private headers shouldn't be replaced by the umbrella header?
Presumably because they're not exported by it...

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

712

Add a test that private headers are not replaced by the umbrella?

[dgoldman]

Support private headers + private umbrellas

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

333

In case the splitFrameworkHeaderPath fails we shouldn't cache the failure. But it's unexpected and shouldn't happen in practice now that I've added the private headers support, so flipped it to remove the entry if it happens.

341

Just went ahead and added proper support for privateheaders since it wasn't too much more work. They should have a separate umbrella header (or none), see https://clang.llvm.org/docs/Modules.html#private-module-map-files

[dgoldman]

In D117056#3256538, @sammccall wrote:

This looks good now, only blocker is dropping the change to LookupFile somehow.

Is it valuable to land this before the imminent 14 branch cut? I think it's OK but as an indexing change a crash has the possibility to be really disruptive, and most of the rest of us aren't regularly dogfooding this on Mac.
I'd suggest we only land it before the branch if you feel like you can test this on a reasonable variety of code in the next few weeks. WDYT?

SGTM to wait until after, the cut is in 2 weeks right? That should be fine

[dgoldman]

Split up HeaderSearch change

[sammccall]

Thanks! LG with a couple of optional nits.
Cut should be on 2022-02-01 or thereabouts, best to land after that if it's not a problem.

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

315

nit: move next to first use

320

now that we're filling in a couple of fields for a whole cache entry, probably worth pulling out a function just for that. That will also naturally eliminate the duplicate returns here.

[dgoldman]

Formatting fixes

[dgoldman]

Rebase

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

 }
 
 } // namespace
 
 // Encapsulates decisions about how to record header paths in the index,
 // including filename normalization, URI conversion etc.
 // Expensive checks are cached internally.
 class SymbolCollector::HeaderFileURICache {
+  struct FrameworkUmbrellaSpelling {
+    // Spelling for the public umbrella header, e.g. <Foundation/Foundation.h>
+    llvm::Optional<std::string> PublicHeader;
+    // Spelling for the private umbrella header, e.g.
+    // <Foundation/Foundation_Private.h>
+    llvm::Optional<std::string> PrivateHeader;
+  };
   // Weird double-indirect access to PP, which might not be ready yet when
   // HeaderFiles is created but will be by the time it's used.
   // (IndexDataConsumer::setPreprocessor can happen before or after initialize)
   Preprocessor *&PP;
   const SourceManager &SM;
   const CanonicalIncludes *Includes;
   llvm::StringRef FallbackDir;
   llvm::DenseMap<const FileEntry *, const std::string *> CacheFEToURI;
   llvm::StringMap<std::string> CachePathToURI;
   llvm::DenseMap<FileID, llvm::StringRef> CacheFIDToInclude;
+  llvm::StringMap<std::string> CachePathToFrameworkSpelling;
+  llvm::StringMap<FrameworkUmbrellaSpelling> CacheFrameworkToUmbrellaHeaderSpelling;
 
 public:
   HeaderFileURICache(Preprocessor *&PP, const SourceManager &SM,
                      const SymbolCollector::Options &Opts)
       : PP(PP), SM(SM), Includes(Opts.Includes), FallbackDir(Opts.FallbackDir) {
   }
 
   // Returns a canonical URI for the file \p FE.
if (R.second) {
              "If the VFS can't make paths absolute, a FallbackDir must be "
              "provided");
       llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
       R.first->second = URI::create(AbsPath).toString();
     }
     return R.first->second;
   }
 
+  struct FrameworkHeaderPath {
+    // Path to the framework directory containing the Headers/PrivateHeaders
+    // directories  e.g. /Frameworks/Foundation.framework/
+    llvm::StringRef HeadersParentDir;
+    // Subpath relative to the Headers or PrivateHeaders dir, e.g. NSObject.h
+    // Note: This is NOT relative to the `HeadersParentDir`.
+    llvm::StringRef HeaderSubpath;
+    // Whether this header is under the PrivateHeaders dir
+    bool IsPrivateHeader;
+  };
+
+  llvm::Optional<FrameworkHeaderPath>
+  splitFrameworkHeaderPath(llvm::StringRef Path) {
+    using namespace llvm::sys;
+    path::reverse_iterator I = path::rbegin(Path);
+    path::reverse_iterator Prev = I;
+    path::reverse_iterator E = path::rend(Path);
+    while (I != E) {
+      if (*I == "Headers") {
+        FrameworkHeaderPath HeaderPath;
+        HeaderPath.HeadersParentDir = Path.substr(0, I - E);
+        HeaderPath.HeaderSubpath = Path.substr(Prev - E);
+        return HeaderPath;
+      }
+      if (*I == "PrivateHeaders") {
+        FrameworkHeaderPath HeaderPath;
+        HeaderPath.HeadersParentDir = Path.substr(0, I - E);
+        HeaderPath.HeaderSubpath = Path.substr(Prev - E);
+        HeaderPath.IsPrivateHeader = true;
+        return HeaderPath;
+      }
+      Prev = I;
+      ++I;
+    }
+    // Unexpected, must not be a framework header.
+    return llvm::None;
+  }
+
+  // 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.
+  llvm::Optional<std::string>
+  getFrameworkUmbrellaSpelling(llvm::StringRef Framework,
+                               SrcMgr::CharacteristicKind HeadersDirKind,
+                               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;
+    }
+    llvm::vfs::Status Status;
+    bool IsSystem = isSystem(HeadersDirKind);
+    SmallString<256> UmbrellaPath(HeaderPath.HeadersParentDir);
+    llvm::sys::path::append(UmbrellaPath, "Headers", Framework + ".h");
+
+    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);
+    }
+
+    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);
+    }
+    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>.
+  llvm::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 llvm::None;
+    }
+    auto DirKind = HS.getFileDirFlavor(FE);
+    if (auto UmbrellaSpelling =
+            getFrameworkUmbrellaSpelling(Framework, DirKind, 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();
+    return llvm::StringRef(*CachedHeaderSpelling);
+  }
+
   llvm::StringRef getIncludeHeaderUncached(FileID FID) {
     const FileEntry *FE = SM.getFileEntryForID(FID);
     if (!FE || FE->getName().empty())
       return "";
     llvm::StringRef Filename = FE->getName();
     // If a file is mapped by canonical headers, use that mapping, regardless
     // of whether it's an otherwise-good header (header guards etc).
     if (Includes) {
       llvm::StringRef Canonical = Includes->mapHeader(Filename);
       if (!Canonical.empty()) {
         // If we had a mapping, always use it.
         if (Canonical.startswith("<") || Canonical.startswith("\""))
           return Canonical;
         return toURI(Canonical);
       }
     }
+    // Framework headers are spelled as <FrameworkName/Foo.h>, not
+    // "path/FrameworkName.framework/Headers/Foo.h".
+    auto &HS = PP->getHeaderSearchInfo();
+    if (const auto *HFI = HS.getExistingFileInfo(FE, /*WantExternal*/ false))
+      if (!HFI->Framework.empty())
+        if (auto Spelling =
+                getFrameworkHeaderIncludeSpelling(FE, HFI->Framework, HS))
+          return *Spelling;
+
     if (!isSelfContainedHeader(FE, FID, PP->getSourceManager(),
                                PP->getHeaderSearchInfo())) {
       // A .inc or .def file is often included into a real header to define
       // symbols (e.g. LLVM tablegen files).
       if (Filename.endswith(".inc") || Filename.endswith(".def"))
         // Don't use cache reentrantly due to iterator invalidation.
         return getIncludeHeaderUncached(SM.getFileID(SM.getIncludeLoc(FID)));
       // Conservatively refuse to insert #includes to files without guards.
       return "";

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

TEST_F(SymbolCollectorTest, ObjCClassExtensions) {
   runSymbolCollector(Header.code(), Main.code(),
                      {"-xobjective-c++", "-Wno-objc-root-class"});
   EXPECT_THAT(Symbols,
               UnorderedElementsAre(
                   AllOf(QName("Cat"), DeclRange(Header.range("catdecl"))),
                   QName("Cat::meow"), QName("Cat::pur")));
 }
 
+TEST_F(SymbolCollectorTest, ObjCFrameworkIncludeHeader) {
+  CollectorOpts.CollectIncludePath = true;
+  auto FrameworksPath = testPath("Frameworks/");
+  std::string FrameworkHeader = R"(
+    __attribute((objc_root_class))
+    @interface NSObject
+    @end
+  )";
+  InMemoryFileSystem->addFile(
+      testPath("Frameworks/Foundation.framework/Headers/NSObject.h"), 0,
+      llvm::MemoryBuffer::getMemBuffer(FrameworkHeader));
+  std::string PrivateFrameworkHeader = R"(
+    #import <Foundation/Foundation.h>
+
+    @interface PrivateClass : NSObject
+    @end
+  )";
+  InMemoryFileSystem->addFile(
+      testPath("Frameworks/Foundation.framework/Headers/NSObject.h"), 0,
+      llvm::MemoryBuffer::getMemBuffer(FrameworkHeader));
+  InMemoryFileSystem->addFile(
+      testPath("Frameworks/Foundation.framework/PrivateHeaders/NSObject+Private.h"), 0,
+      llvm::MemoryBuffer::getMemBuffer(PrivateFrameworkHeader));
+
+  std::string Header = R"(
+    #import <Foundation/NSObject.h>
+    #import <Foundation/NSObject+Private.h>
+
+    @interface Container : NSObject
+    @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("PrivateClass"),
+                                 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"))));
+
+  runSymbolCollector(Header, Main,
+                     {"-iframework", 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"))));
+}
+
 TEST_F(SymbolCollectorTest, Locations) {
   Annotations Header(R"cpp(
     // Declared in header, defined in main.
     extern int $xdecl[[X]];
     class $clsdecl[[Cls]];
     void $printdecl[[print]]();
 
     // Declared in header, defined nowhere.
     extern int $zdecl[[Z]];
 
     void $foodecl[[fo\
 o]]();
   )cpp");
   Annotations Main(R"cpp(
     int $xdef[[X]] = 42;
     class $clsdef[[Cls]] {};
     void $printdef[[print]]() {}
 
     // Declared/defined in main only.
     int $ydecl[[Y]];
   )cpp");
   runSymbolCollector(Header.code(), Main.code());
   EXPECT_THAT(Symbols,
               UnorderedElementsAre(
                   AllOf(QName("X"), DeclRange(Header.range("xdecl")),
                         DefRange(Main.range("xdef"))),
                   AllOf(QName("Cls"), DeclRange(Header.range("clsdecl")),
                         DefRange(Main.range("clsdef"))),
                   AllOf(QName("print"), DeclRange(Header.range("printdecl")),
                         DefRange(Main.range("printdef"))),
                   AllOf(QName("Z"), DeclRange(Header.range("zdecl"))),
                   AllOf(QName("foo"), DeclRange(Header.range("foodecl"))),
                   AllOf(QName("Y"), DeclRange(Main.range("ydecl")))));

 
 TEST_F(SymbolCollectorTest, SpelledReferences) {
   struct {
     llvm::StringRef Header;
     llvm::StringRef Main;
     llvm::StringRef TargetSymbolName;
   } TestCases[] = {
     {
       R"cpp(
         struct Foo;
         #define MACRO Foo
       )cpp",
       R"cpp(
         struct $spelled[[Foo]] {
           $spelled[[Foo]]();
           ~$spelled[[Foo]]();
         };