From: Kadir Cetinkaya Date: Mon, 15 Feb 2021 08:00:49 +0000 (+0100) Subject: [clangd] Treat paths case-insensitively depending on the platform X-Git-Tag: llvmorg-14-init~14974 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=ecea7218fb9b994b26471e9877851cdb51a5f1d4;p=platform%2Fupstream%2Fllvm.git [clangd] Treat paths case-insensitively depending on the platform 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: - Creating regexes with case-insensitive matching on those platforms. - Introducing a new pathIsAncestor helper, which performs checks in a case-correct manner where needed. Differential Revision: https://reviews.llvm.org/D96690 --- diff --git a/clang-tools-extra/clangd/ConfigCompile.cpp b/clang-tools-extra/clangd/ConfigCompile.cpp index 8682cae..dadc578 100644 --- a/clang-tools-extra/clangd/ConfigCompile.cpp +++ b/clang-tools-extra/clangd/ConfigCompile.cpp @@ -31,6 +31,7 @@ #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" @@ -101,9 +102,11 @@ struct FragmentCompiler { // Normalized Fragment::SourceInfo::Directory. std::string FragmentDirectory; - llvm::Optional compileRegex(const Located &Text) { + llvm::Optional + compileRegex(const Located &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); @@ -195,9 +198,15 @@ struct FragmentCompiler { 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>(); 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()) { @@ -218,7 +227,7 @@ struct FragmentCompiler { auto PathExclude = std::make_unique>(); 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()) { @@ -349,7 +358,8 @@ struct FragmentCompiler { 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 (P.Path.empty() || !pathStartsWith(Spec.MountPoint, P.Path, + llvm::sys::path::Style::posix)) return; C.Index.External = Spec; // Disable background indexing for the files under the mountpoint. diff --git a/clang-tools-extra/clangd/support/Path.cpp b/clang-tools-extra/clangd/support/Path.cpp index f72d000..6fc74b9 100644 --- a/clang-tools-extra/clangd/support/Path.cpp +++ b/clang-tools-extra/clangd/support/Path.cpp @@ -7,24 +7,33 @@ //===----------------------------------------------------------------------===// #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 pathStartsWith(PathRef Ancestor, PathRef Path, + llvm::sys::path::Style Style) { + assert(llvm::sys::path::is_absolute(Ancestor, Style) && + llvm::sys::path::is_absolute(Path, 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 diff --git a/clang-tools-extra/clangd/support/Path.h b/clang-tools-extra/clangd/support/Path.h index 4029031..938d7d7 100644 --- a/clang-tools-extra/clangd/support/Path.h +++ b/clang-tools-extra/clangd/support/Path.h @@ -10,8 +10,14 @@ #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SUPPORT_PATH_H #include "llvm/ADT/StringRef.h" +#include "llvm/Support/Path.h" #include +/// Whether current platform treats paths case insensitively. +#if defined(_WIN32) || defined(__APPLE__) +#define CLANGD_PATH_CASE_INSENSITIVE +#endif + namespace clang { namespace clangd { @@ -28,6 +34,12 @@ using PathRef = llvm::StringRef; std::string maybeCaseFoldPath(PathRef Path); bool pathEqual(PathRef, PathRef); +/// Checks if \p Ancestor is a proper ancestor of \p Path. This is just a +/// smarter lexical prefix match, e.g: foo/bar/baz doesn't start with foo/./bar. +/// Both \p Ancestor and \p Path must be absolute. +bool pathStartsWith( + PathRef Ancestor, PathRef Path, + llvm::sys::path::Style Style = llvm::sys::path::Style::native); } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/unittests/CMakeLists.txt b/clang-tools-extra/clangd/unittests/CMakeLists.txt index a0ecf0a..b16a443 100644 --- a/clang-tools-extra/clangd/unittests/CMakeLists.txt +++ b/clang-tools-extra/clangd/unittests/CMakeLists.txt @@ -105,6 +105,7 @@ add_unittest(ClangdUnitTests ClangdTests support/FunctionTests.cpp support/MarkupTests.cpp support/MemoryTreeTests.cpp + support/PathTests.cpp support/ThreadingTests.cpp support/TestTracer.cpp support/TraceTests.cpp diff --git a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp index 4b1da20..d9aa171 100644 --- a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp +++ b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp @@ -99,6 +99,25 @@ TEST_F(ConfigCompileTests, Condition) { 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) { @@ -406,6 +425,23 @@ TEST_F(ConfigCompileTests, ExternalBlockMountPoint) { 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); + FooPath = llvm::sys::path::convert_to_slash(FooPath); + 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 diff --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp b/clang-tools-extra/clangd/unittests/RenameTests.cpp index b6eccf0..5e16640 100644 --- a/clang-tools-extra/clangd/unittests/RenameTests.cpp +++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp @@ -1125,7 +1125,7 @@ TEST(RenameTest, IndexMergeMainFile) { 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"; diff --git a/clang-tools-extra/clangd/unittests/support/PathTests.cpp b/clang-tools-extra/clangd/unittests/support/PathTests.cpp new file mode 100644 index 0000000..26b999d --- /dev/null +++ b/clang-tools-extra/clangd/unittests/support/PathTests.cpp @@ -0,0 +1,36 @@ +//===-- 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(pathStartsWith("/foo", "/foo")); + EXPECT_TRUE(pathStartsWith("/foo/", "/foo")); + + EXPECT_FALSE(pathStartsWith("/foo", "/fooz")); + EXPECT_FALSE(pathStartsWith("/foo/", "/fooz")); + + EXPECT_TRUE(pathStartsWith("/foo", "/foo/bar")); + EXPECT_TRUE(pathStartsWith("/foo/", "/foo/bar")); + +#ifdef CLANGD_PATH_CASE_INSENSITIVE + EXPECT_TRUE(pathStartsWith("/fOo", "/foo/bar")); + EXPECT_TRUE(pathStartsWith("/foo", "/fOo/bar")); +#else + EXPECT_FALSE(pathStartsWith("/fOo", "/foo/bar")); + EXPECT_FALSE(pathStartsWith("/foo", "/fOo/bar")); +#endif +} +} // namespace +} // namespace clangd +} // namespace clang