Revert "[Clang] [Diagnostics] Simplify filenames that contain '..'" (#148367)

Sirraide · web-flow · commit 7b43c6c6a729 · 2025-07-12T15:13:22.000+02:00
Revert #143520 for now since it’s causing issues for people who are using symlinks and prefer to preserve the original path (i.e. looks like we’ll have to make this configurable after all; I just need to figure out how to pass `-no-canonical-prefixes` down through the driver); I’m planning to refactor this a bit and reland it in a few days.
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/file-filter-symlinks.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/file-filter-symlinks.cpp
@@ -12,9 +12,8 @@
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='header\.h' %s -- -I %t 2>&1 | FileCheck --check-prefix=CHECK_HEADER %s
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='header\.h' -quiet %s -- -I %t 2>&1 | FileCheck --check-prefix=CHECK_HEADER %s
 
-// `-header-filter` operates on the actual file path that the user provided in
-// the #include directive; however, Clang's path name simplification causes the
-// path to be printed in canonicalised form here.
+// Check that `-header-filter` operates on the same file paths as paths in
+// diagnostics printed by ClangTidy.
 #include "dir1/dir2/../header_alias.h"
-// CHECK_HEADER_ALIAS: dir1/header.h:1:11: warning: single-argument constructors
+// CHECK_HEADER_ALIAS: dir1/dir2/../header_alias.h:1:11: warning: single-argument constructors
 // CHECK_HEADER-NOT: warning:
diff --git a/clang/include/clang/Basic/SourceManager.h b/clang/include/clang/Basic/SourceManager.h
@@ -824,12 +824,6 @@ class SourceManager : public RefCountedBase<SourceManager> {
 
   mutable std::unique_ptr<SrcMgr::SLocEntry> FakeSLocEntryForRecovery;
 
-  /// Cache for filenames used in diagnostics. See 'getNameForDiagnostic()'.
-  mutable llvm::StringMap<StringRef> DiagNames;
-
-  /// Allocator for absolute/short names.
-  mutable llvm::BumpPtrAllocator DiagNameAlloc;
-
   /// Lazily computed map of macro argument chunks to their expanded
   /// source location.
   using MacroArgsMap = std::map<unsigned, SourceLocation>;
@@ -1854,16 +1848,6 @@ class SourceManager : public RefCountedBase<SourceManager> {
   /// \return Location of the top-level macro caller.
   SourceLocation getTopMacroCallerLoc(SourceLocation Loc) const;
 
-  /// Retrieve the name of a file suitable for diagnostics.
-  // FIXME: Passing in the DiagnosticOptions here is a workaround for the
-  // fact that installapi does some weird things with DiagnosticsEngines,
-  // which causes the 'Diag' member of SourceManager (or at least the
-  // DiagnosticsOptions member thereof) to be a dangling reference
-  // sometimes. We should probably fix that or decouple the two classes
-  // to avoid this issue entirely.
-  StringRef getNameForDiagnostic(StringRef Filename,
-                                 const DiagnosticOptions &Opts) const;
-
 private:
   friend class ASTReader;
   friend class ASTWriter;
diff --git a/clang/lib/Basic/SourceManager.cpp b/clang/lib/Basic/SourceManager.cpp
@@ -2390,75 +2390,3 @@ SourceManagerForFile::SourceManagerForFile(StringRef FileName,
   assert(ID.isValid());
   SourceMgr->setMainFileID(ID);
 }
-
-StringRef
-SourceManager::getNameForDiagnostic(StringRef Filename,
-                                    const DiagnosticOptions &Opts) const {
-  OptionalFileEntryRef File = getFileManager().getOptionalFileRef(Filename);
-  if (!File)
-    return Filename;
-
-  bool SimplifyPath = [&] {
-    if (Opts.AbsolutePath)
-      return true;
-
-    // Try to simplify paths that contain '..' in any case since paths to
-    // standard library headers especially tend to get quite long otherwise.
-    // Only do that for local filesystems though to avoid slowing down
-    // compilation too much.
-    if (!File->getName().contains(".."))
-      return false;
-
-    // If we're not on Windows, check if we're on a network file system and
-    // avoid simplifying the path in that case since that can be slow. On
-    // Windows, the check for a local filesystem is already slow, so skip it.
-#ifndef _WIN32
-    if (!llvm::sys::fs::is_local(File->getName()))
-      return false;
-#endif
-
-    return true;
-  }();
-
-  if (!SimplifyPath)
-    return Filename;
-
-  // This may involve computing canonical names, so cache the result.
-  StringRef &CacheEntry = DiagNames[Filename];
-  if (!CacheEntry.empty())
-    return CacheEntry;
-
-  // We want to print a simplified absolute path, i. e. without "dots".
-  //
-  // The hardest part here are the paths like "<part1>/<link>/../<part2>".
-  // On Unix-like systems, we cannot just collapse "<link>/..", because
-  // paths are resolved sequentially, and, thereby, the path
-  // "<part1>/<part2>" may point to a different location. That is why
-  // we use FileManager::getCanonicalName(), which expands all indirections
-  // with llvm::sys::fs::real_path() and caches the result.
-  //
-  // On the other hand, it would be better to preserve as much of the
-  // original path as possible, because that helps a user to recognize it.
-  // real_path() expands all links, which sometimes too much. Luckily,
-  // on Windows we can just use llvm::sys::path::remove_dots(), because,
-  // on that system, both aforementioned paths point to the same place.
-  SmallString<256> TempBuf;
-#ifdef _WIN32
-  TempBuf = File->getName();
-  llvm::sys::fs::make_absolute(TempBuf);
-  llvm::sys::path::native(TempBuf);
-  llvm::sys::path::remove_dots(TempBuf, /* remove_dot_dot */ true);
-#else
-  TempBuf = getFileManager().getCanonicalName(*File);
-#endif
-
-  // In some cases, the resolved path may actually end up being longer (e.g.
-  // if it was originally a relative path), so just retain whichever one
-  // ends up being shorter.
-  if (!Opts.AbsolutePath && TempBuf.size() > Filename.size())
-    CacheEntry = Filename;
-  else
-    CacheEntry = TempBuf.str().copy(DiagNameAlloc);
-
-  return CacheEntry;
-}
diff --git a/clang/lib/Frontend/SARIFDiagnostic.cpp b/clang/lib/Frontend/SARIFDiagnostic.cpp
@@ -163,7 +163,36 @@ SARIFDiagnostic::addDiagnosticLevelToRule(SarifRule Rule,
 
 llvm::StringRef SARIFDiagnostic::emitFilename(StringRef Filename,
                                               const SourceManager &SM) {
-  return SM.getNameForDiagnostic(Filename, DiagOpts);
+  if (DiagOpts.AbsolutePath) {
+    auto File = SM.getFileManager().getOptionalFileRef(Filename);
+    if (File) {
+      // We want to print a simplified absolute path, i. e. without "dots".
+      //
+      // The hardest part here are the paths like "<part1>/<link>/../<part2>".
+      // On Unix-like systems, we cannot just collapse "<link>/..", because
+      // paths are resolved sequentially, and, thereby, the path
+      // "<part1>/<part2>" may point to a different location. That is why
+      // we use FileManager::getCanonicalName(), which expands all indirections
+      // with llvm::sys::fs::real_path() and caches the result.
+      //
+      // On the other hand, it would be better to preserve as much of the
+      // original path as possible, because that helps a user to recognize it.
+      // real_path() expands all links, which is sometimes too much. Luckily,
+      // on Windows we can just use llvm::sys::path::remove_dots(), because,
+      // on that system, both aforementioned paths point to the same place.
+#ifdef _WIN32
+      SmallString<256> TmpFilename = File->getName();
+      llvm::sys::fs::make_absolute(TmpFilename);
+      llvm::sys::path::native(TmpFilename);
+      llvm::sys::path::remove_dots(TmpFilename, /* remove_dot_dot */ true);
+      Filename = StringRef(TmpFilename.data(), TmpFilename.size());
+#else
+      Filename = SM.getFileManager().getCanonicalName(*File);
+#endif
+    }
+  }
+
+  return Filename;
 }
 
 /// Print out the file/line/column information and include trace.
diff --git a/clang/lib/Frontend/TextDiagnostic.cpp b/clang/lib/Frontend/TextDiagnostic.cpp
@@ -738,7 +738,39 @@ void TextDiagnostic::printDiagnosticMessage(raw_ostream &OS,
 }
 
 void TextDiagnostic::emitFilename(StringRef Filename, const SourceManager &SM) {
-  OS << SM.getNameForDiagnostic(Filename, DiagOpts);
+#ifdef _WIN32
+  SmallString<4096> TmpFilename;
+#endif
+  if (DiagOpts.AbsolutePath) {
+    auto File = SM.getFileManager().getOptionalFileRef(Filename);
+    if (File) {
+      // We want to print a simplified absolute path, i. e. without "dots".
+      //
+      // The hardest part here are the paths like "<part1>/<link>/../<part2>".
+      // On Unix-like systems, we cannot just collapse "<link>/..", because
+      // paths are resolved sequentially, and, thereby, the path
+      // "<part1>/<part2>" may point to a different location. That is why
+      // we use FileManager::getCanonicalName(), which expands all indirections
+      // with llvm::sys::fs::real_path() and caches the result.
+      //
+      // On the other hand, it would be better to preserve as much of the
+      // original path as possible, because that helps a user to recognize it.
+      // real_path() expands all links, which sometimes too much. Luckily,
+      // on Windows we can just use llvm::sys::path::remove_dots(), because,
+      // on that system, both aforementioned paths point to the same place.
+#ifdef _WIN32
+      TmpFilename = File->getName();
+      llvm::sys::fs::make_absolute(TmpFilename);
+      llvm::sys::path::native(TmpFilename);
+      llvm::sys::path::remove_dots(TmpFilename, /* remove_dot_dot */ true);
+      Filename = StringRef(TmpFilename.data(), TmpFilename.size());
+#else
+      Filename = SM.getFileManager().getCanonicalName(*File);
+#endif
+    }
+  }
+
+  OS << Filename;
 }
 
 /// Print out the file/line/column information and include trace.
diff --git a/clang/test/Frontend/absolute-paths.c b/clang/test/Frontend/absolute-paths.c
@@ -8,9 +8,9 @@
 
 #include "absolute-paths.h"
 
-// Check that the bogus prefix we added is stripped out even if absolute paths
-// are disabled.
-// NORMAL-NOT: SystemHeaderPrefix
+// Check whether the diagnostic from the header above includes the dummy
+// directory in the path.
+// NORMAL: SystemHeaderPrefix
 // ABSOLUTE-NOT: SystemHeaderPrefix
 // CHECK: warning: non-void function does not return a value
 
diff --git a/clang/test/Frontend/simplify-paths.c b/clang/test/Frontend/simplify-paths.c
