[clang][lex] Remove `PPCallbacks::FileNotFound()`

[jansvoboda11]

Summary

The purpose of the FileNotFound preprocessor callback was to add the ability to recover from failed header lookups. This was to support downstream project.

However, injecting additional search path while performing header search can invalidate currently used iterators/references to DirectoryLookup in Preprocessor and HeaderSearch.

The downstream project ended up maintaining a separate patch to further tweak the functionality. Since we don't have any upstream users nor open source downstream users, I'd like to remove this callback for good to prevent future misuse. I doubt there are any actual downstream users, since the functionality is definitely broken at the moment.

[ahoppen]

Assuming that this is indeed never used, removing dead code always sounds good to me.

[Hahnfeld]

Hello, sorry for the late heads-up, but this functionality is used by ROOT: https://github.com/root-project/root/blob/f58cccf5ce7fd67894c7fd9e9e74d3f37bc1acba/core/metacling/src/TClingCallbacks.cxx#L282 Any chance of bringing this back?

[jansvoboda11]

In D119708#4059254, @Hahnfeld wrote:

Hello, sorry for the late heads-up, but this functionality is used by ROOT: https://github.com/root-project/root/blob/f58cccf5ce7fd67894c7fd9e9e74d3f37bc1acba/core/metacling/src/TClingCallbacks.cxx#L282 Any chance of bringing this back?

Hi. Your downstream callback never fills in RecoveryPath and always returns false, meaning it will never reach the unsafe block of code I had issue with: HeaderInfo.AddSearchPath(DL, isAngled).
Technically, we could bring back a safe/trimmed-down version of this callback that doesn't take the out-parameter and returns void. But I don't think it's the right call, since that would essentially re-introduce dead code to upstream.
This is probably best kept downstream, unless there are other downstream users.

[Hahnfeld]

In D119708#4060336, @jansvoboda11 wrote:

In D119708#4059254, @Hahnfeld wrote:

Hello, sorry for the late heads-up, but this functionality is used by ROOT: https://github.com/root-project/root/blob/f58cccf5ce7fd67894c7fd9e9e74d3f37bc1acba/core/metacling/src/TClingCallbacks.cxx#L282 Any chance of bringing this back?

Hi. Your downstream callback never fills in RecoveryPath and always returns false, meaning it will never reach the unsafe block of code I had issue with: HeaderInfo.AddSearchPath(DL, isAngled).

This is true, but it provides us a hook to do something in case a file is not found. For ROOT, we "extend" the #include syntax and allow "modifiers" at the end. Then we do something special (completely on our own) for things like #include "myfile.C+" (note the plus).

Technically, we could bring back a safe/trimmed-down version of this callback that doesn't take the out-parameter and returns void. But I don't think it's the right call, since that would essentially re-introduce dead code to upstream.
This is probably best kept downstream, unless there are other downstream users.

Sure, but how would we do that without a hook? At the moment, we try very hard to reduce the number of patches that we carry for Clang (which is also why parts of Cling are upstreamed into clang-repl).

[v.g.vassilev]

In D119708#4060336, @jansvoboda11 wrote:

In D119708#4059254, @Hahnfeld wrote:

Hello, sorry for the late heads-up, but this functionality is used by ROOT: https://github.com/root-project/root/blob/f58cccf5ce7fd67894c7fd9e9e74d3f37bc1acba/core/metacling/src/TClingCallbacks.cxx#L282 Any chance of bringing this back?

Hi. Your downstream callback never fills in RecoveryPath and always returns false, meaning it will never reach the unsafe block of code I had issue with: HeaderInfo.AddSearchPath(DL, isAngled).
Technically, we could bring back a safe/trimmed-down version of this callback that doesn't take the out-parameter and returns void. But I don't think it's the right call, since that would essentially re-introduce dead code to upstream.
This is probably best kept downstream, unless there are other downstream users.

+1 for bringing back that piece of code because it seems an interesting event to listen to anyways. Would you be more comfortable if we provide also a test case and relevant comments explaining the use-case?

[jansvoboda11]

If the plan is to eventually upstream that part of Cling, I'm fine with re-adding a safe version of this API.

[Hahnfeld]

In D119708#4062632, @jansvoboda11 wrote:

If the plan is to eventually upstream that part of Cling, I'm fine with re-adding a safe version of this API.

I'll be honest here and say that I personally find it unlikely that this part of ROOT will be upstreamed, it is very specific and also there for backwards-compatibility in a very specific case. Nevertheless, I understand that Clang should offer its clients a rich API to get all information they need, so I put together https://reviews.llvm.org/D142196 with a hopefully "safe" version of a FileNotFound callback, that optionally even supports silently skipping this file. This would provide exactly what we need.

Another possibility that I considered is extending InclusionDirective and always call it, also for missing files. However, I find this much less clear and we already have FileSkipped...

clang-tools-extra/clangd/ParsedAST.cpp

for (const auto &Inc : Includes) {
           Inc.Written.front() == '<',
           syntax::FileRange(SM, SynthesizedFilenameTok.getLocation(),
                             SynthesizedFilenameTok.getEndLoc())
               .toCharRange(SM),
           FE, "SearchPath", "RelPath",
           /*Imported=*/nullptr, Inc.FileKind);
       if (File)
         Delegate->FileSkipped(*File, SynthesizedFilenameTok, Inc.FileKind);
-      else {
-        llvm::SmallString<1> UnusedRecovery;
-        Delegate->FileNotFound(WrittenFilename, UnusedRecovery);
-      }
     }
   }
 
   const std::vector<Inclusion> Includes;
   PPCallbacks *Delegate;
   const SourceManager &SM;
   Preprocessor &PP;
   std::vector<syntax::Token> MainFileTokens;

clang-tools-extra/pp-trace/PPCallbacksTracker.h

public:
 
   // Overridden callback functions.
 
   void FileChanged(SourceLocation Loc, PPCallbacks::FileChangeReason Reason,
                    SrcMgr::CharacteristicKind FileType,
                    FileID PrevFID = FileID()) override;
   void FileSkipped(const FileEntryRef &SkippedFile, const Token &FilenameTok,
                    SrcMgr::CharacteristicKind FileType) override;
-  bool FileNotFound(llvm::StringRef FileName,
-                    llvm::SmallVectorImpl<char> &RecoveryPath) override;
   void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
                           llvm::StringRef FileName, bool IsAngled,
                           CharSourceRange FilenameRange, const FileEntry *File,
                           llvm::StringRef SearchPath,
                           llvm::StringRef RelativePath, const Module *Imported,
                           SrcMgr::CharacteristicKind FileType) override;
   void moduleImport(SourceLocation ImportLoc, ModuleIdPath Path,
                     const Module *Imported) override;

clang-tools-extra/pp-trace/PPCallbacksTracker.cpp

void PPCallbacksTracker::FileSkipped(const FileEntryRef &SkippedFile,
                                      const Token &FilenameTok,
                                      SrcMgr::CharacteristicKind FileType) {
   beginCallback("FileSkipped");
   appendArgument("ParentFile", &SkippedFile.getFileEntry());
   appendArgument("FilenameTok", FilenameTok);
   appendArgument("FileType", FileType, CharacteristicKindStrings);
 }
 
-// Callback invoked whenever an inclusion directive results in a
-// file-not-found error.
-bool
-PPCallbacksTracker::FileNotFound(llvm::StringRef FileName,
-                                 llvm::SmallVectorImpl<char> &RecoveryPath) {
-  beginCallback("FileNotFound");
-  appendFilePathArgument("FileName", FileName);
-  return false;
-}
-
 // Callback invoked whenever an inclusion directive of
 // any kind (#include, #import, etc.) has been processed, regardless
 // of whether the inclusion will actually result in an inclusion.
 void PPCallbacksTracker::InclusionDirective(
     SourceLocation HashLoc, const Token &IncludeTok, llvm::StringRef FileName,
     bool IsAngled, CharSourceRange FilenameRange, const FileEntry *File,
     llvm::StringRef SearchPath, llvm::StringRef RelativePath,
     const Module *Imported, SrcMgr::CharacteristicKind FileType) {

clang/include/clang/Lex/PPCallbacks.h

public:
   ///
   /// \param FilenameTok The file name token in \#include "FileName" directive
   /// or macro expanded file name token from \#include MACRO(PARAMS) directive.
   /// Note that FilenameTok contains corresponding quotes/angles symbols.
   virtual void FileSkipped(const FileEntryRef &SkippedFile,
                            const Token &FilenameTok,
                            SrcMgr::CharacteristicKind FileType) {}
 
-  /// Callback invoked whenever an inclusion directive results in a
-  /// file-not-found error.
-  ///
-  /// \param FileName The name of the file being included, as written in the
-  /// source code.
-  ///
-  /// \param RecoveryPath If this client indicates that it can recover from
-  /// this missing file, the client should set this as an additional header
-  /// search patch.
-  ///
-  /// \returns true to indicate that the preprocessor should attempt to recover
-  /// by adding \p RecoveryPath as a header search path.
-  virtual bool FileNotFound(StringRef FileName,
-                            SmallVectorImpl<char> &RecoveryPath) {
-    return false;
-  }
-
   /// Callback invoked whenever an inclusion directive of
   /// any kind (\c \#include, \c \#import, etc.) has been processed, regardless
   /// of whether the inclusion will actually result in an inclusion.
   ///
   /// \param HashLoc The location of the '#' that starts the inclusion
   /// directive.
   ///
   /// \param IncludeTok The token that indicates the kind of inclusion
public:
   }
 
   void FileSkipped(const FileEntryRef &SkippedFile, const Token &FilenameTok,
                    SrcMgr::CharacteristicKind FileType) override {
     First->FileSkipped(SkippedFile, FilenameTok, FileType);
     Second->FileSkipped(SkippedFile, FilenameTok, FileType);
   }
 
-  bool FileNotFound(StringRef FileName,
-                    SmallVectorImpl<char> &RecoveryPath) override {
-    return First->FileNotFound(FileName, RecoveryPath) ||
-           Second->FileNotFound(FileName, RecoveryPath);
-  }
-
   void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
                           StringRef FileName, bool IsAngled,
                           CharSourceRange FilenameRange, const FileEntry *File,
                           StringRef SearchPath, StringRef RelativePath,
                           const Module *Imported,
                           SrcMgr::CharacteristicKind FileType) override {
     First->InclusionDirective(HashLoc, IncludeTok, FileName, IsAngled,
                               FilenameRange, File, SearchPath, RelativePath,

clang/lib/Lex/PPDirectives.cpp

Optional<FileEntryRef> Preprocessor::LookupHeaderIncludeOrImport(
   Optional<FileEntryRef> File = LookupFile(
       FilenameLoc, LookupFilename,
       isAngled, LookupFrom, LookupFromFile, CurDir,
       Callbacks ? &SearchPath : nullptr, Callbacks ? &RelativePath : nullptr,
       &SuggestedModule, &IsMapped, &IsFrameworkFound);
   if (File)
     return File;
 
-  if (Callbacks) {
-    // Give the clients a chance to recover.
-    SmallString<128> RecoveryPath;
-    if (Callbacks->FileNotFound(Filename, RecoveryPath)) {
-      if (auto DE = FileMgr.getOptionalDirectoryRef(RecoveryPath)) {
-        // Add the recovery path to the list of search paths.
-        DirectoryLookup DL(*DE, SrcMgr::C_User, false);
-        HeaderInfo.AddSearchPath(DL, isAngled);
-
-        // Try the lookup again, skipping the cache.
-        Optional<FileEntryRef> File = LookupFile(
-            FilenameLoc,
-            LookupFilename, isAngled,
-            LookupFrom, LookupFromFile, CurDir, nullptr, nullptr,
-            &SuggestedModule, &IsMapped, /*IsFrameworkFound=*/nullptr,
-            /*SkipCache*/ true);
-        if (File)
-          return File;
-      }
-    }
-  }
-
   if (SuppressIncludeNotFoundError)
     return None;
 
   // If the file could not be located and it was included via angle
   // brackets, we can attempt a lookup as though it were a quoted path to
   // provide the user with a possible fixit.
   if (isAngled) {
     Optional<FileEntryRef> File = LookupFile(