[clangd] Treat paths case-insensitively depending on the platform

[kadircet]

Summary

Path{Match,Exclude} and MountPoint were checking paths case-sensitively
on all platforms, as with other features, this was causing problems on
windows. Since users can have capital drive letters on config files, but
editors might lower-case them.

This patch addresses that issue by:

• Normalizing all the paths being passed to ConfigProvider in ClangdServer.
• Storing MountPoint and regexes used for pathmatching in case-folded manner.

[sammccall]

Thanks for the important fix.

This contains a mixture of changing comparison-of-paths to be case insensitive, and case-normalizing stored paths so we can use case-sensitive comparisons.

I think we should stick to *only* changing comparison-of-path logic as much as possible, because:

• both approaches are ad-hoc and error prone - forgetting to fold/insensitive-compare will introduce a bug
• it's more obvious where to fix a sensitive-compare, and it has fewer unexpected effects
• in many configs we only ever see one case for a given file, case-folding can introduce bugs here where insensitive-comparison cannot
• there's a risk we won't be case-preserving, which can be annoying or really matter (e.g. on a mac volume that *is* case sensitive)

clang-tools-extra/clangd/ConfigCompile.cpp

201

I'm starting to think Path.h should export a CLANGD_PATH_INSENSITIVE macro, this condition is in a bunch of places. (particularly tests)

I nearly did this last time and came to the conclusion it wasn't quite enough :-\

[kadircet]

• Don't case fold stored strings, or the ones passing interface boundaries
• Define a pathIsAncestor helper instead and use that for comparison of MountPoint
• Expose a CLANGD_PATH_CASE_INSENSITIVE macro, which is defined whenever the platform treats paths case-insensitively

[sammccall]

Nice, thank you!

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

22

we should decide what the preconditions are here

Could assert both are absolute paths, this places a burden on callers.

I do like the idea this is "just" a smarter lexical startswith. (I'd consider pathStartsWith rather than pathIsAncestor for this reason)
In this case we should doc it (e.g. foo/./bar is not an ancestor of foo/bar/baz) and return false when Ancestor is empty (rather than crash)

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

18

(we could conditionally define this as either 0 or 1 and then use it for regex flags without ifdef, up to you)

[kadircet]

• s/pathIsAncestor/pathStartsWith
• Fix tests

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

22

oops nice catch. going with the latter option.

clang-tools-extra/clangd/ConfigCompile.cpp

 #include "CompileCommands.h"
 #include "Config.h"
 #include "ConfigFragment.h"
 #include "ConfigProvider.h"
 #include "Diagnostics.h"
 #include "Features.inc"
 #include "TidyProvider.h"
 #include "support/Logger.h"
+#include "support/Path.h"
 #include "support/Trace.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Support/Error.h"
FragmentCompiler(CompiledFragmentImpl &Out, DiagnosticCallback D,
                    llvm::SourceMgr *SM)
       : Out(Out), Diagnostic(D), SourceMgr(SM) {}
   CompiledFragmentImpl &Out;
   DiagnosticCallback Diagnostic;
   llvm::SourceMgr *SourceMgr;
   // Normalized Fragment::SourceInfo::Directory.
   std::string FragmentDirectory;
 
-  llvm::Optional<llvm::Regex> compileRegex(const Located<std::string> &Text) {
+  llvm::Optional<llvm::Regex>
+  compileRegex(const Located<std::string> &Text,
+               llvm::Regex::RegexFlags Flags = llvm::Regex::NoFlags) {
     std::string Anchored = "^(" + *Text + ")$";
-    llvm::Regex Result(Anchored);
+    llvm::Regex Result(Anchored, Flags);
     std::string RegexError;
     if (!Result.isValid(RegexError)) {
       diag(Error, "Invalid regex " + Anchored + ": " + RegexError, Text.Range);
       return llvm::None;
     }
     return Result;
   }
 
void compile(Fragment &&F) {
     compile(std::move(F.Index));
     compile(std::move(F.Diagnostics));
   }
 
   void compile(Fragment::IfBlock &&F) {
     if (F.HasUnrecognizedCondition)
       Out.Conditions.push_back([&](const Params &) { return false; });
 
+#ifdef CLANGD_PATH_CASE_INSENSITIVE
+    llvm::Regex::RegexFlags Flags = llvm::Regex::IgnoreCase;
+#else
+    llvm::Regex::RegexFlags Flags = llvm::Regex::NoFlags;
+#endif
+
     auto PathMatch = std::make_unique<std::vector<llvm::Regex>>();
     for (auto &Entry : F.PathMatch) {
-      if (auto RE = compileRegex(Entry))
+      if (auto RE = compileRegex(Entry, Flags))
         PathMatch->push_back(std::move(*RE));
     }
     if (!PathMatch->empty()) {
       Out.Conditions.push_back(
           [PathMatch(std::move(PathMatch)),
            FragmentDir(FragmentDirectory)](const Params &P) {
             if (P.Path.empty())
               return false;
             llvm::StringRef Path = configRelative(P.Path, FragmentDir);
             // Ignore the file if it is not nested under Fragment.
             if (Path.empty())
               return false;
             return llvm::any_of(*PathMatch, [&](const llvm::Regex &RE) {
               return RE.match(Path);
             });
           });
     }
 
     auto PathExclude = std::make_unique<std::vector<llvm::Regex>>();
     for (auto &Entry : F.PathExclude) {
-      if (auto RE = compileRegex(Entry))
+      if (auto RE = compileRegex(Entry, Flags))
         PathExclude->push_back(std::move(*RE));
     }
     if (!PathExclude->empty()) {
       Out.Conditions.push_back(
           [PathExclude(std::move(PathExclude)),
            FragmentDir(FragmentDirectory)](const Params &P) {
             if (P.Path.empty())
               return false;
if ((**External.MountPoint).empty()) {
       return;
     }
     auto AbsPath = makeAbsolute(std::move(*External.MountPoint), "MountPoint",
                                 llvm::sys::path::Style::posix);
     if (!AbsPath)
       return;
     Spec.MountPoint = std::move(*AbsPath);
     Out.Apply.push_back([Spec(std::move(Spec))](const Params &P, Config &C) {
-      if (!P.Path.startswith(Spec.MountPoint))
+      if (!pathIsAncestor(Spec.MountPoint, P.Path,
+                          llvm::sys::path::Style::posix))
         return;
       C.Index.External = Spec;
       // Disable background indexing for the files under the mountpoint.
       // Note that this will overwrite statements in any previous fragments
       // (including the current one).
       C.Index.Background = Config::BackgroundPolicy::Skip;
     });
   }

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

 //===--- Path.h - Helper typedefs --------------------------------*- 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SUPPORT_PATH_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SUPPORT_PATH_H
 
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Path.h"
 #include <string>
 
+/// Whether current platform treats paths case insensitively.
+#if defined(_WIN32) || defined(__APPLE__)
+#define CLANGD_PATH_CASE_INSENSITIVE
+#endif
+
 namespace clang {
 namespace clangd {
 
 /// A typedef to represent a file path. Used solely for more descriptive
 /// signatures.
 using Path = std::string;
 /// A typedef to represent a ref to file path. Used solely for more descriptive
 /// signatures.
 using PathRef = llvm::StringRef;
 
 // For platforms where paths are case-insensitive (but case-preserving),
 // we need to do case-insensitive comparisons and use lowercase keys.
 // FIXME: Make Path a real class with desired semantics instead.
 std::string maybeCaseFoldPath(PathRef Path);
 bool pathEqual(PathRef, PathRef);
 
+/// Checks if \p Ancestor is a proper ancestor of \p Path.
+bool pathIsAncestor(
+    PathRef Ancestor, PathRef Path,
+    llvm::sys::path::Style Style = llvm::sys::path::Style::native);
 } // namespace clangd
 } // namespace clang
 
 #endif

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

 //===--- Path.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 "support/Path.h"
+#include "llvm/Support/Path.h"
 namespace clang {
 namespace clangd {
 
-std::string maybeCaseFoldPath(PathRef Path) {
-#if defined(_WIN32) || defined(__APPLE__)
-  return Path.lower();
-#else
-  return std::string(Path);
-#endif
-}
+#ifdef CLANGD_PATH_CASE_INSENSITIVE
+std::string maybeCaseFoldPath(PathRef Path) { return Path.lower(); }
+bool pathEqual(PathRef A, PathRef B) { return A.equals_lower(B); }
+#else  // NOT CLANGD_PATH_CASE_INSENSITIVE
+std::string maybeCaseFoldPath(PathRef Path) { return Path.str(); }
+bool pathEqual(PathRef A, PathRef B) { return A == B; }
+#endif // CLANGD_PATH_CASE_INSENSITIVE
 
-bool pathEqual(PathRef A, PathRef B) {
-#if defined(_WIN32) || defined(__APPLE__)
-  return A.equals_lower(B);
-#else
-  return A == B;
-#endif
+bool pathIsAncestor(PathRef Ancestor, PathRef Path,
+                    llvm::sys::path::Style Style) {
+  // If ancestor ends with a separator drop that, so that we can match /foo/ as
+  // a parent of /foo.
+  if (llvm::sys::path::is_separator(Ancestor.back(), Style))
+    Ancestor = Ancestor.drop_back();
+  // Ensure Path starts with Ancestor.
+  if (!pathEqual(Ancestor, Path.take_front(Ancestor.size())))
+    return false;
+  Path = Path.drop_front(Ancestor.size());
+  // Then make sure either two paths are equal or Path has a separator
+  // afterwards.
+  return Path.empty() || llvm::sys::path::is_separator(Path.front(), Style);
 }
-
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/unittests/CMakeLists.txt

add_unittest(ClangdUnitTests ClangdTests
   XRefsTests.cpp
   ${CMAKE_CURRENT_BINARY_DIR}/DecisionForestRuntimeTest.cpp
 
   support/CancellationTests.cpp
   support/ContextTests.cpp
   support/FunctionTests.cpp
   support/MarkupTests.cpp
   support/MemoryTreeTests.cpp
+  support/PathTests.cpp
   support/ThreadingTests.cpp
   support/TestTracer.cpp
   support/TraceTests.cpp
 
   tweaks/AddUsingTests.cpp
   tweaks/AnnotateHighlightingsTests.cpp
   tweaks/DefineInlineTests.cpp
   tweaks/DefineOutlineTests.cpp

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

TEST_F(ConfigCompileTests, Condition) {
   EXPECT_THAT(Diags.Diagnostics.front().Message, StartsWith("Invalid regex"));
 
   // Valid regex and unknown key.
   Frag = {};
   Frag.If.HasUnrecognizedCondition = true;
   Frag.If.PathMatch.emplace_back("ba*r");
   EXPECT_FALSE(compileAndApply());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+
+  // Only matches case-insensitively.
+  Frag = {};
+  Frag.If.PathMatch.emplace_back("B*R");
+  EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+#ifdef CLANGD_PATH_CASE_INSENSITIVE
+  EXPECT_TRUE(compileAndApply());
+#else
+  EXPECT_FALSE(compileAndApply());
+#endif
+
+  Frag = {};
+  Frag.If.PathExclude.emplace_back("B*R");
+  EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+#ifdef CLANGD_PATH_CASE_INSENSITIVE
+  EXPECT_FALSE(compileAndApply());
+#else
+  EXPECT_TRUE(compileAndApply());
+#endif
 }
 
 TEST_F(ConfigCompileTests, CompileCommands) {
   Frag.CompileFlags.Add.emplace_back("-foo");
   Frag.CompileFlags.Remove.emplace_back("--include-directory=");
   std::vector<std::string> Argv = {"clang", "-I", "bar/", "a.cc"};
   EXPECT_TRUE(compileAndApply());
   EXPECT_THAT(Conf.CompileFlags.Edits, SizeIs(2));
TEST_F(ConfigCompileTests, ExternalBlockMountPoint) {
   BazPath = testPath("foo/baz.h", llvm::sys::path::Style::posix);
   BazPath = llvm::sys::path::convert_to_slash(BazPath);
   Parm.Path = BazPath;
   Frag = GetFrag("", FooPath.c_str());
   compileAndApply();
   ASSERT_THAT(Diags.Diagnostics, IsEmpty());
   ASSERT_TRUE(Conf.Index.External);
   EXPECT_THAT(Conf.Index.External->MountPoint, FooPath);
+
+  // Only matches case-insensitively.
+  BazPath = testPath("fOo/baz.h", llvm::sys::path::Style::posix);
+  BazPath = llvm::sys::path::convert_to_slash(BazPath);
+  Parm.Path = BazPath;
+
+  FooPath = testPath("FOO/", llvm::sys::path::Style::posix);
+  Frag = GetFrag("", FooPath.c_str());
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+#ifdef CLANGD_PATH_CASE_INSENSITIVE
+  ASSERT_TRUE(Conf.Index.External);
+  EXPECT_THAT(Conf.Index.External->MountPoint, FooPath);
+#else
+  ASSERT_FALSE(Conf.Index.External);
+#endif
 }
 } // namespace
 } // namespace config
 } // namespace clangd
 } // namespace clang

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

EXPECT_THAT(Results.GlobalChanges[Main].asTextEdits(),
               ElementsAre(newText("xPrime")));
 
   // Sanity check: we do expect to see index results!
   TU.Filename = "other.cc";
   Results = Rename(TU.index().get());
   EXPECT_THAT(Results.GlobalChanges.keys(),
               UnorderedElementsAre(Main, testPath("other.cc")));
 
-#if defined(_WIN32) || defined(__APPLE__)
+#ifdef CLANGD_PATH_CASE_INSENSITIVE
   // On case-insensitive systems, no duplicates if AST vs index case differs.
   // https://github.com/clangd/clangd/issues/665
   TU.Filename = "MAIN.CC";
   Results = Rename(TU.index().get());
   EXPECT_THAT(Results.GlobalChanges.keys(), ElementsAre(Main));
   EXPECT_THAT(Results.GlobalChanges[Main].asTextEdits(),
               ElementsAre(newText("xPrime")));
 #endif

clang-tools-extra/clangd/unittests/support/PathTests.cpp

+//===-- PathTests.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 "support/Path.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+TEST(PathTests, IsAncestor) {
+  EXPECT_TRUE(pathIsAncestor("/foo", "/foo"));
+  EXPECT_TRUE(pathIsAncestor("/foo/", "/foo"));
+
+  EXPECT_FALSE(pathIsAncestor("/foo", "/fooz"));
+  EXPECT_FALSE(pathIsAncestor("/foo/", "/fooz"));
+
+  EXPECT_TRUE(pathIsAncestor("/foo", "/foo/bar"));
+  EXPECT_TRUE(pathIsAncestor("/foo/", "/foo/bar"));
+
+#ifdef CLANGD_PATH_CASE_INSENSITIVE
+  EXPECT_TRUE(pathIsAncestor("/fOo", "/foo/bar"));
+  EXPECT_TRUE(pathIsAncestor("/foo", "/fOo/bar"));
+#else
+  EXPECT_FALSE(pathIsAncestor("/fOo", "/foo/bar"));
+  EXPECT_FALSE(pathIsAncestor("/foo", "/fOo/bar"));
+#endif
+}
+} // namespace
+} // namespace clangd
+} // namespace clang