[clangd] Handle case insensitive file systems in header/source switch

[kadircet]

Summary

Fixes https://github.com/clangd/clangd/issues/1043

[sammccall]

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

88

This is clever, but I don't think it's a good idea.

It's very surprising given the contract and historical behavior of this function, and can lead to spurious test failures in some legitimate uses, e.g in IncludeCleanerTests:

// Note we deduped the names as _number_ of <built-in>s is uninteresting.
EXPECT_THAT(ReferencedFileNames.keys(),
            UnorderedElementsAre("<built-in>", "<scratch space>",
                                 testPath("macros.h")));

(Not sure why this isn't failing, luck? Maybe you'll get more failures by adding more possible case variations?)
I don't think the extra verbosity to make such tests resilient to case changes is an improvement.

The paths coming from testRoot get propagated around a lot, so lots of strings start having unpredictable case (which is the point!). Nevertheless I'm not confident many of them will be effectively tested, because the paths compared need to originate through independent calls to testRoot() rather than e.g. both resulting from relative paths resolved against a CWD set to testRoot() called once.

[kadircet]

Add tests specifically and postpone change in testRoot

[kadircet]

• Adjust after tests on windows

[kadircet]

• rename patch

[kadircet]

• Match path ignoring case.

[kadircet]

ping

[sammccall]

clang-tools-extra/clangd/support/Path.h

50 ↗

(On Diff #414326)

This function is a bit of a trap.

c.f. pathStartsWith which understands path semantics: pathStartsWith("a/b", "a/bc") is false.

This function returns true/false based on paths semantics, but the stripping is lexical:

• pathConsumeFront("a/b/c", "a/b/") is "c"
• pathConsumeFront("a/b/c", "a/b") is "/c", a very different path.

It's safe where called here because we know testRoot() ends in a slash, but I'd probably inline it for that reason - it's not as reusable as it looks.

Up to you, though.

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

277

mac is the same as windows here I think.

This should probably be #ifdef CLANGD_PATH_CASE_INSENSITIVE if possible?

If you want to be really clever, you could keep the test on all platforms and just #ifdef the assertions to give opposite results. We don't want tests to pass if we are case insensitive everywhere, right?

281

are the inline functions essential to this test?
Even with context I was struggling a bit to understand what this is testing, so it'd be nice to cut down a few elements

294

either a comment or a variable name explaining what we're doing here?

// Providing the "wrong"-case drive letter in the query should still find the file.

Why uppercase the drive letter only and not the whole filename? Then we could run the test on mac too

295

May be nice to have an assertion that HeaderAbsPath != testPath(TU.HeaderFilename), so that we're not spuriously passing because testRoot() is "C:\" or "\\something"

[kadircet]

• Address comments

[kadircet]

clang-tools-extra/clangd/support/Path.h

50 ↗

(On Diff #414326)

fair point, moved into TestFS.cpp.

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

281

yes, we need majority of the symbols to be defined in the header, so that the heuristic picks the header as the implementation file (if the header isn't ignored due to case-sensitive check of filepaths). adding a comment though.

[kadircet]

• Address comments for real

clang-tools-extra/clangd/HeaderSourceSwitch.cpp

 //===--- HeaderSourceSwitch.cpp - --------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "HeaderSourceSwitch.h"
 #include "AST.h"
 #include "SourceCode.h"
 #include "index/SymbolCollector.h"
 #include "support/Logger.h"
+#include "support/Path.h"
 #include "clang/AST/Decl.h"
 
 namespace clang {
 namespace clangd {
 
 llvm::Optional<Path> getCorrespondingHeaderOrSource(
     PathRef OriginalFile, llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS) {
   llvm::StringRef SourceExtensions[] = {".cpp", ".c", ".cc", ".cxx",
llvm::Optional<Path> getCorrespondingHeaderOrSource(PathRef OriginalFile,
   // Find all symbols present in the original file.
   for (const auto *D : getIndexableLocalDecls(AST)) {
     if (auto ID = getSymbolID(D))
       Request.IDs.insert(ID);
   }
   llvm::StringMap<int> Candidates; // Target path => score.
   auto AwardTarget = [&](const char *TargetURI) {
     if (auto TargetPath = URI::resolve(TargetURI, OriginalFile)) {
-      if (*TargetPath != OriginalFile) // exclude the original file.
+      if (!pathEqual(*TargetPath, OriginalFile)) // exclude the original file.
         ++Candidates[*TargetPath];
     } else {
       elog("Failed to resolve URI {0}: {1}", TargetURI, TargetPath.takeError());
     }
   };
   // If we switch from a header, we are looking for the implementation
   // file, so we use the definition loc; otherwise we look for the header file,
   // we use the decl loc;

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

 //===--- HeaderSourceSwitchTests.cpp - ---------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "HeaderSourceSwitch.h"
 
 #include "SyncAPI.h"
 #include "TestFS.h"
 #include "TestTU.h"
 #include "index/MemIndex.h"
+#include "support/Path.h"
 #include "llvm/ADT/None.h"
+#include "llvm/Testing/Support/SupportHelpers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
 TEST(HeaderSourceSwitchTest, FileHeuristic) {
TEST(HeaderSourceSwitchTest, ClangdServerIntegration) {
   auto Options = ClangdServer::optsForTest();
   Options.BuildDynamicSymbolIndex = true;
   ClangdServer Server(CDB, FS, Options);
   runAddDocument(Server, CppPath, FileContent);
   EXPECT_EQ(HeaderPath,
             *llvm::cantFail(runSwitchHeaderSource(Server, CppPath)));
 }
 
+TEST(HeaderSourceSwitchTest, CaseSensitivity) {
+  TestTU TU = TestTU::withCode("void foo() {}");
+  // Define more symbols in the header than the source file to trick heuristics
+  // into picking the header as source file, if the matching for header file
+  // path fails.
+  TU.HeaderCode = R"cpp(
+  inline void bar1() {}
+  inline void bar2() {}
+  void foo();)cpp";
+  // Give main file and header different base names to make sure file system
+  // heuristics don't work.
+  TU.Filename = "Source.cpp";
+  TU.HeaderFilename = "Header.h";
+
+  auto Index = TU.index();
+  TU.Code = std::move(TU.HeaderCode);
+  TU.HeaderCode.clear();
+  auto AST = TU.build();
+
+  // Provide a different-cased filename in the query than what we have in the
+  // index, check if we can still find the source file, which defines less
+  // symbols than the header.
+  auto HeaderAbsPath = testPath("HEADER.H");
+  // We expect the heuristics to pick:
+  // - header on case sensitive file systems, because the HeaderAbsPath doesn't
+  //   match what we've seen through index.
+  // - source on case insensitive file systems, as the HeaderAbsPath would match
+  //   the filename in index.
+#ifdef CLANGD_PATH_CASE_INSENSITIVE
+  EXPECT_THAT(getCorrespondingHeaderOrSource(HeaderAbsPath, AST, Index.get()),
+              llvm::ValueIs(testing::StrCaseEq(testPath(TU.Filename))));
+#else
+  EXPECT_THAT(getCorrespondingHeaderOrSource(HeaderAbsPath, AST, Index.get()),
+              llvm::ValueIs(testing::StrCaseEq(testPath(TU.HeaderFilename))));
+#endif
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang

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

 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Path.h"
 
 namespace clang {
 namespace clangd {
 
+namespace {
+
+// Tries to strip \p Prefix from beginning of \p Path. Returns true on success.
+// If \p Prefix doesn't match, leaves \p Path untouched and returns false.
+bool pathConsumeFront(PathRef &Path, PathRef Prefix) {
+  if (!pathStartsWith(Prefix, Path))
+    return false;
+  Path = Path.drop_front(Prefix.size());
+  return true;
+}
+} // namespace
+
 llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
 buildTestFS(llvm::StringMap<std::string> const &Files,
             llvm::StringMap<time_t> const &Timestamps) {
   llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> MemFS(
       new llvm::vfs::InMemoryFileSystem);
   MemFS->setCurrentWorkingDirectory(testRoot());
   for (auto &FileAndContents : Files) {
     llvm::StringRef File = FileAndContents.first();
MockCompilationDatabase::getCompileCommand(PathRef File) const {
   return {tooling::CompileCommand(Directory != llvm::StringRef()
                                       ? Directory
                                       : llvm::sys::path::parent_path(File),
                                   FileName, std::move(CommandLine), "")};
 }
 
 const char *testRoot() {
 #ifdef _WIN32
   return "C:\\clangd-test";
 #else
   return "/clangd-test";
 #endif
 }
 
 std::string testPath(PathRef File, llvm::sys::path::Style Style) {
   assert(llvm::sys::path::is_relative(File) && "FileName should be relative");
 

 /// /clangd-test/x.h.
 class TestScheme : public URIScheme {
 public:
   static const char *Scheme;
 
   llvm::Expected<std::string>
   getAbsolutePath(llvm::StringRef /*Authority*/, llvm::StringRef Body,
                   llvm::StringRef HintPath) const override {
-    if (!HintPath.empty() && !HintPath.startswith(testRoot()))
+    if (!HintPath.empty() && !pathStartsWith(testRoot(), HintPath))
       return error("Hint path is not empty and doesn't start with {0}: {1}",
                    testRoot(), HintPath);
     if (!Body.consume_front("/"))
       return error("Body of an unittest: URI must start with '/'");
     llvm::SmallString<16> Path(Body.begin(), Body.end());
     llvm::sys::path::native(Path);
     return testPath(Path);
   }
 
   llvm::Expected<URI>
   uriFromAbsolutePath(llvm::StringRef AbsolutePath) const override {
-    llvm::StringRef Body = AbsolutePath;
-    if (!Body.consume_front(testRoot()))
+    if (!pathConsumeFront(AbsolutePath, testRoot()))
       return error("{0} does not start with {1}", AbsolutePath, testRoot());
 
     return URI(Scheme, /*Authority=*/"",
-               llvm::sys::path::convert_to_slash(Body));
+               llvm::sys::path::convert_to_slash(AbsolutePath));
   }
 };
 
 const char *TestScheme::Scheme = "unittest";
 
 static URISchemeRegistry::Add<TestScheme> X(TestScheme::Scheme, "Test schema");
 
 volatile int UnittestSchemeAnchorSource = 0;
 
 } // namespace clangd
 } // namespace clang