[PATCH] D66254: Correct include suggestion when search path includes symlink

[Ben Jackson via Phabricator via cfe-commits]

puremourning created this revision.
puremourning added a reviewer: sammccall.
Herald added subscribers: cfe-commits, kadircet, ilya-biryukov.
Herald added a project: clang.
puremourning added a comment.

FYI this fixes the issue in https://github.com/clangd/clangd/issues/124

I haven't added new tests to HeaderSearchTest.cc because the InMemoryFileSystem doesn't support symlinks and I didn't want to try and implement that for this patch. Let me know thoughts on alternative ways to regression test it.


When a header search path contained a symlink, the suggested include
spelling could be wrong, as the files to insert provided by clangd are
always canonical (i.e. with the symlinks removed), but the search paths
(which come from flags), might not be. As files from Sema also come from
the VFS, they are also canonical, I think.

When trying to suggest the spelling of the include to insert, we should
always compare canonical paths. This corrects the include suggestion in
this case.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66254

Files:
  clang/include/clang/Lex/HeaderSearch.h
  clang/lib/Lex/HeaderSearch.cpp


Index: clang/lib/Lex/HeaderSearch.cpp
===================================================================
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -1723,7 +1723,13 @@
     if (!WorkingDir.empty() && !path::is_absolute(Dir))
       fs::make_absolute(WorkingDir, DirPath);
     path::remove_dots(DirPath, /*remove_dot_dot=*/true);
-    Dir = DirPath;
+    // Find the canonical absolute path, as that's what we expect in File
+    auto& FS = FileMgr.getVirtualFileSystem();
+    SmallString<4096> CanonicalNameBuf;
+    if (!FS.getRealPath(DirPath, CanonicalNameBuf))
+      Dir = CanonicalNameBuf;
+    else
+      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) {
Index: clang/include/clang/Lex/HeaderSearch.h
===================================================================
--- clang/include/clang/Lex/HeaderSearch.h
+++ clang/include/clang/Lex/HeaderSearch.h
@@ -727,6 +727,7 @@
   /// MainFile location, if none of the include search directories were prefix
   /// of File.
   ///
+  /// \param File A canonical absolute path to the file to be included.
   /// \param WorkingDir If non-empty, this will be prepended to search directory
   /// paths that are relative.
   std::string suggestPathToFileForDiagnostics(llvm::StringRef File,


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D66254.215235.patch
Type: text/x-patch
Size: 1419 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190814/7bd35852/attachment.bin>