release/21.x: [clangd] Use resolved path when checking AngledHeaders/QuotedHeaders in IncludeInserter (#148371)

[llvmbot]

Backport ff0093c

Requested by: @HighCommander4

[llvmbot]

@HighCommander4 What do you think about merging this PR to the release branch?

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clangd

Author: None (llvmbot)

Changes

Backport ff0093c

Requested by: @HighCommander4

---

Full diff: https://github.com/llvm/llvm-project/pull/152659.diff

5 Files Affected:

• (modified) clang-tools-extra/clangd/ConfigFragment.h (+2-2)
• (modified) clang-tools-extra/clangd/Headers.cpp (+4-3)
• (modified) clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp (+2-2)
• (modified) clang-tools-extra/clangd/unittests/HeadersTests.cpp (+11)
• (modified) clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h (+1-1)

diff --git a/clang-tools-extra/clangd/ConfigFragment.h b/clang-tools-extra/clangd/ConfigFragment.h
index 9e00dbc9dc90a..c5488c2378a1e 100644
--- a/clang-tools-extra/clangd/ConfigFragment.h
+++ b/clang-tools-extra/clangd/ConfigFragment.h
@@ -315,7 +315,7 @@ struct Fragment {
     /// AngledHeaders (i.e. a header matches a regex in both QuotedHeaders and
     /// AngledHeaders), system headers use <> and non-system headers use "".
     /// These can match any suffix of the header file in question.
-    /// Matching is performed against the header text, not its absolute path
+    /// Matching is performed against the absolute path of the header
     /// within the project.
     std::vector<Located<std::string>> QuotedHeaders;
     /// List of regexes for headers that should always be included with a
@@ -323,7 +323,7 @@ struct Fragment {
     /// AngledHeaders (i.e. a header matches a regex in both QuotedHeaders and
     /// AngledHeaders), system headers use <> and non-system headers use "".
     /// These can match any suffix of the header file in question.
-    /// Matching is performed against the header text, not its absolute path
+    /// Matching is performed against the absolute path of the header
     /// within the project.
     std::vector<Located<std::string>> AngledHeaders;
   };
diff --git a/clang-tools-extra/clangd/Headers.cpp b/clang-tools-extra/clangd/Headers.cpp
index 87fd261b906e6..b9d67cc6a1602 100644
--- a/clang-tools-extra/clangd/Headers.cpp
+++ b/clang-tools-extra/clangd/Headers.cpp
@@ -304,16 +304,17 @@ IncludeInserter::calculateIncludePath(const HeaderFile &InsertedHeader,
   // FIXME: should we allow (some limited number of) "../header.h"?
   if (llvm::sys::path::is_absolute(Suggested))
     return std::nullopt;
+  auto HeaderPath = llvm::sys::path::convert_to_slash(InsertedHeader.File);
   bool IsAngled = false;
   for (auto &Filter : AngledHeaders) {
-    if (Filter(Suggested)) {
+    if (Filter(HeaderPath)) {
       IsAngled = true;
       break;
     }
   }
   bool IsQuoted = false;
   for (auto &Filter : QuotedHeaders) {
-    if (Filter(Suggested)) {
+    if (Filter(HeaderPath)) {
       IsQuoted = true;
       break;
     }
@@ -324,7 +325,7 @@ IncludeInserter::calculateIncludePath(const HeaderFile &InsertedHeader,
     if (IsAngled && IsQuoted) {
       elog("Header '{0}' matches both quoted and angled regexes, default will "
            "be used.",
-           Suggested);
+           HeaderPath);
     }
     IsAngled = IsAngledByDefault;
   }
diff --git a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
index b7c64c7a06745..3c107504e6253 100644
--- a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -938,7 +938,7 @@ TEST(CompletionTest, IncludeInsertionRespectsQuotedAngledConfig) {
   {
     Config C;
     C.Style.AngledHeaders.push_back(
-        [](auto header) { return header == "bar.h"; });
+        [](auto header) { return header.contains("bar.h"); });
     WithContextValue WithCfg(Config::Key, std::move(C));
     Results = completions(TU, Test.point(), {Sym});
     EXPECT_THAT(Results.Completions,
@@ -947,7 +947,7 @@ TEST(CompletionTest, IncludeInsertionRespectsQuotedAngledConfig) {
   {
     Config C;
     C.Style.QuotedHeaders.push_back(
-        [](auto header) { return header == "bar.h"; });
+        [](auto header) { return header.contains("bar.h"); });
     WithContextValue WithCfg(Config::Key, std::move(C));
     Results = completions(TU, Test.point(), {Sym});
     EXPECT_THAT(Results.Completions,
diff --git a/clang-tools-extra/clangd/unittests/HeadersTests.cpp b/clang-tools-extra/clangd/unittests/HeadersTests.cpp
index 751383e3b4650..440582e14239a 100644
--- a/clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ b/clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -344,6 +344,17 @@ TEST_F(HeadersTest, ShortenIncludesInSearchPathBracketed) {
   EXPECT_EQ(calculate(BarHeader), "<sub/bar.h>");
 }
 
+TEST_F(HeadersTest, ShortenIncludesInSearchPathBracketedFilterByFullPath) {
+  // The filter receives the full path of the header, so it is able to filter by
+  // the parent directory, even if it is part of the include search path
+  AngledHeaders.push_back([](auto Path) {
+    llvm::Regex Pattern("sub/.*");
+    return Pattern.match(Path);
+  });
+  std::string BarHeader = testPath("sub/bar.h");
+  EXPECT_EQ(calculate(BarHeader), "<bar.h>");
+}
+
 TEST_F(HeadersTest, ShortenedIncludeNotInSearchPath) {
   std::string BarHeader =
       llvm::sys::path::convert_to_slash(testPath("sub-2/bar.h"));
diff --git a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h
index 2888e25226755..057b92c147047 100644
--- a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h
+++ b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h
@@ -136,7 +136,7 @@ struct Header {
   }
   StringRef verbatim() const { return std::get<Verbatim>(Storage); }
 
-  /// For phiscal files, either absolute path or path relative to the execution
+  /// For physical files, either absolute path or path relative to the execution
   /// root. Otherwise just the spelling without surrounding quotes/brackets.
   llvm::StringRef resolvedPath() const;
 

[HighCommander4]

@HighCommander4 What do you think about merging this PR to the release branch?

Since I'm the requester this should probably be ok'd by @kadircet

[HighCommander4]

Copying over backport rationale from #152658:

This is a small, low-risk change that tweaks how one of clangd's features (IncludeInserter) interprets a configuration option (AnglesHeaders/QuotedHeaders).

Including this in 21.x avoids shipping clangd 21 in a state where two different features interpret the configuration option in different ways (with IncludeInserter using the spelled path and include-cleaner using the resolved path).

[kadircet]

thanks a lot, yes this totally makes sense to backport!

[github-actions]

@HighCommander4 (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.