[clang][HeaderSuggestion] Handle the case of dotdot with an absolute path

[kadircet]

Summary

Include insertion in clangd was inserting absolute paths when the
include directory was an absolute path with a double dot. This patch makes sure
double dots are handled both with absolute and relative paths.

[sammccall]

Well done!
I never managed to track this one down, this was really annoying.

[ormris]

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

220 ↗

(On Diff #195751)

Quick nit: This test won't work on Windows as it only tests for POSIX-style path separators. You could easily add a Windows ifdef, though.

[sammccall]

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

220 ↗

(On Diff #195751)

testPath() returns an appropriate native path, and calculate() returns an #include string (which always uses /).
The "../" on line 217 looks a little more suspicious, but I think the clang driver might silently Do What I Mean here.
Are you seeing test failures?

[ormris]

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

220 ↗

(On Diff #195751)

Yes, here's the output.

[ RUN      ] HeadersTest.ShortenedInclude
C:\llvm\tools\clang\tools\extra\unittests\clangd\HeadersTests.cpp(220): error:       Expected: calculate(BarHeader)
      Which is: "\"sub\\bar.h\""
To be equal to: "\"sub/bar.h\""

[sammccall]

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

220 ↗

(On Diff #195751)

Hmm, that suggests the test is catching a real bug in the code: we should be inserting paths with forward slashes even on Windows.

Looking at the test cases, i have the sinking feeling we've always been doing this and never noticed as no test case has a slash in the final #include. (And none of the main clangd devs are primarily using Windows)

Thank you for pointing this out!

@kadircet: we should either fix this before this patch lands, or ifdef it until we can fix.

[kadircet]

Sent out D60995, hopefully it should fix the issue. Will wait until that lands.

[sammccall]

I think this introduced a bug: if your search path has /usr/bin/../include/c++/v1, then you'll end up with FileEntrys with Name like /usr/bin/../include/c++/v1/__utility/move.h. Calling the FileEntry overload of suggestPathToFileForDiagnostics won't return the relative path, because it will strip dots from the dir path but not the fileentry's Name.

Fix is probably to strip dots from both, I guess.

cfe/trunk/lib/Lex/HeaderSearch.cpp

std::string HeaderSearch::suggestPathToFileForDiagnostics(
 
   for (unsigned I = 0; I != SearchDirs.size(); ++I) {
     // FIXME: Support this search within frameworks and header maps.
     if (!SearchDirs[I].isNormalDir())
       continue;
 
     StringRef Dir = SearchDirs[I].getDir()->getName();
     llvm::SmallString<32> DirPath(Dir.begin(), Dir.end());
-    if (!WorkingDir.empty() && !path::is_absolute(Dir)) {
+    if (!WorkingDir.empty() && !path::is_absolute(Dir))
       fs::make_absolute(WorkingDir, DirPath);
-      path::remove_dots(DirPath, /*remove_dot_dot=*/true);
-      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) {
       // '.' components in File are ignored.
       while (NI != NE && *NI == ".")
         ++NI;
       if (NI == NE)
         break;

cfe/trunk/unittests/Lex/HeaderSearchTest.cpp

 TEST_F(HeaderSearchTest, BackSlash) {
   addSearchDir("C:\\x\\y\\");
   EXPECT_EQ(Search.suggestPathToFileForDiagnostics("C:\\x\\y\\z\\t",
                                                    /*WorkingDir=*/""),
             "z/t");
 }
 #endif
 
+TEST_F(HeaderSearchTest, DotDotsWithAbsPath) {
+  addSearchDir("/x/../y/");
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/y/z",
+                                                   /*WorkingDir=*/""),
+            "z");
+}
+
 } // namespace
 } // namespace clang

clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp

 TEST_F(HeadersTest, DoNotInsertIfInSameFile) {
   MainFile = testPath("main.h");
   EXPECT_EQ(calculate(MainFile), "");
 }
 
 TEST_F(HeadersTest, ShortenedInclude) {
   std::string BarHeader = testPath("sub/bar.h");
   EXPECT_EQ(calculate(BarHeader), "\"bar.h\"");
+
+  SearchDirArg = (llvm::Twine("-I") + Subdir + "/..").str();
+  CDB.ExtraClangFlags = {SearchDirArg.c_str()};
+  BarHeader = testPath("sub/bar.h");
+  EXPECT_EQ(calculate(BarHeader), "\"sub/bar.h\"");
 }
 
 TEST_F(HeadersTest, NotShortenedInclude) {
   std::string BarHeader = testPath("sub-2/bar.h");
   EXPECT_EQ(calculate(BarHeader, ""), "\"" + BarHeader + "\"");
 }
 
 TEST_F(HeadersTest, PreferredHeader) {