[include-cleaner] Improve header spelling in the presence of links

[sammccall]

Summary

HeaderSearch uses FileEntry::getName() to determine the best spelling of a
header. FileEntry::getName() is now the name of the *last* retrieved ref.
This means that when FileManager::getFile() hits an existing inode through a new
path, it changes the spelling of that header.

In the absence of explicit logic to track the preferred name(s) of header files,
we should avoid gratuitously calling getFile() with paths different than how
the header was originally included, such as the result of realpath().

The originally-specified path should be fine here:

• if the same filemanager is being used for record/analysis, we'll hit the filename cache
• if a different filemanager is being used e.g. preamble scenario, we should get the same result unless either the working directory has changed (which it shouldn't, else many other things will fail) or the file has gone/changed inode (in which case the old method doesn't work either)

Needless to say this is fragile, but talking to @kadircet offline, it's good
enough for our purposes for now.

[hokein]

Thanks for tracking it down. The solution looks good to me. Since this is a fragile and subtle issue, is it possible to have a unittest for it? If it is not too hard, it would be nice to have a test.

[sammccall]

In D141478#4043134, @hokein wrote:

Thanks for tracking it down. The solution looks good to me. Since this is a fragile and subtle issue, is it possible to have a unittest for it? If it is not too hard, it would be nice to have a test.

I think it's hard to write a test that's worth anything :-(

It can't be a test that poking FileManager doesn't affect spelling, because I didn't fix that.
I can test specifically that getExports() doesn't affect the getName() of relevant fileentries, but if we regress this symptom that's very unlikely to be the mechanism.
An end-to-end test for the symptom needs to be set up in a way that tickles the bad side-effect at exactly the wrong time, and it's hard to predict how to do that in general.

I'm not happy with this patch, but I'm not sure a bad test would help with that :-\

clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h

private:
   //
   // !!NOTE: instead of using a FileEntry* to identify the physical file, we
   // deliberately use the UniqueID to ensure the result is stable across
   // FileManagers (for clangd's preamble and main-file builds).
   llvm::DenseMap<llvm::sys::fs::UniqueID, llvm::StringRef /*VerbatimSpelling*/>
       IWYUPublic;
 
   /// A reverse map from the underlying header to its exporter headers.
-  //
-  //  There's no way to get a FileEntry from a UniqueID, especially when it
-  //  hasn't been opened before. So store the full path and convert it to a
-  //  FileEntry by opening the file again through a FileManager.
+  ///
+  /// There's no way to get a FileEntry from a UniqueID, especially when it
+  /// hasn't been opened before. So store the path and convert it to a
+  /// FileEntry by opening the file again through a FileManager.
+  ///
+  /// We don't use RealPathName, as opening the file through a different name
+  /// changes its preferred name. Clearly this is fragile!
   llvm::DenseMap<llvm::sys::fs::UniqueID,
-                 llvm::SmallVector</*RealPathNames*/ llvm::StringRef>>
+                 llvm::SmallVector</*FileEntry::getName()*/ llvm::StringRef>>
       IWYUExportBy;
 
   /// Contains all non self-contained files detected during the parsing.
   llvm::DenseSet<llvm::sys::fs::UniqueID> NonSelfContainedFiles;
 
   /// Owns the strings.
   llvm::BumpPtrAllocator Arena;
 

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

void checkForExport(FileID IncludingFile, int HashLine,
     auto &Top = ExportStack.back();
     if (Top.SeenAtFile != IncludingFile)
       return;
     // Make sure current include is covered by the export pragma.
     if ((Top.Block && HashLine > Top.SeenAtLine) ||
         Top.SeenAtLine == HashLine) {
       if (IncludedHeader)
         Out->IWYUExportBy[IncludedHeader->getUniqueID()].push_back(
-            Top.FullPath);
+            Top.Path);
       // main-file #include with export pragma should never be removed.
       if (Top.SeenAtFile == SM.getMainFileID())
         Out->ShouldKeep.insert(HashLine);
     }
     if (!Top.Block) // Pop immediately for single-line export pragma.
       ExportStack.pop_back();
   }
 
if (Pragma->consume_front("private")) {
       Out->IWYUPublic.insert({FE->getLastRef().getUniqueID(), PublicHeader});
       return false;
     }
     FileID CommentFID = SM.getFileID(Range.getBegin());
     int CommentLine = SM.getLineNumber(SM.getFileID(Range.getBegin()),
                                        SM.getFileOffset(Range.getBegin()));
     // Record export pragma.
     if (Pragma->startswith("export")) {
-      ExportStack.push_back(
-          {CommentLine, CommentFID,
-           save(SM.getFileEntryForID(CommentFID)->tryGetRealPathName()),
+      ExportStack.push_back({CommentLine, CommentFID,
+                             save(SM.getFileEntryForID(CommentFID)->getName()),
                              false});
     } else if (Pragma->startswith("begin_exports")) {
-      ExportStack.push_back(
-          {CommentLine, CommentFID,
-           save(SM.getFileEntryForID(CommentFID)->tryGetRealPathName()), true});
+      ExportStack.push_back({CommentLine, CommentFID,
+                             save(SM.getFileEntryForID(CommentFID)->getName()),
+                             true});
     } else if (Pragma->startswith("end_exports")) {
       // FIXME: be robust on unmatching cases. We should only pop the stack if
       // the begin_exports and end_exports is in the same file.
       if (!ExportStack.empty()) {
         assert(ExportStack.back().Block);
         ExportStack.pop_back();
       }
     }
private:
   /// Intern table for strings. Contents are on the arena.
   llvm::StringSaver UniqueStrings;
 
   struct ExportPragma {
     // The line number where we saw the begin_exports or export pragma.
     int SeenAtLine = 0; // 1-based line number.
     // The file where we saw the pragma.
     FileID SeenAtFile;
-    // FullPath of the file SeenAtFile.
-    StringRef FullPath;
+    // Name (per FileEntry::getName()) of the file SeenAtFile.
+    StringRef Path;
     // true if it is a block begin/end_exports pragma; false if it is a
     // single-line export pragma.
     bool Block = false;
   };
   // A stack for tracking all open begin_exports or single-line export.
   std::vector<ExportPragma> ExportStack;
 
   struct KeepPragma {