[clangd] Handle absolute/relative path specifications in Config

[kadircet]

Summary

This introduces a mechanism for providers to interpret paths specified
in a fragment either as absolute or relative to fragment location.
This information should be used during compile stage to handle blocks correctly.

[sammccall]

Thanks for doing this!

Main points:

• I think we should have one high-level concept of "the optional associated directory" rather than two low-level ones of "the location of the config file" and "should we use the location to form relative paths"
• We need to be careful about windows vs posix paths

clang-tools-extra/clangd/ConfigCompile.cpp

277

passing a reference to part of this and then separately this by rvalue when the reference is live feels... fragile.

Maybe have compile() assign to a member stringref instead? Or even a string and do the path normalization at that point...

clang-tools-extra/clangd/ConfigFragment.h

94

directory the fragment is associated with? At this level, I don't think we should care whether it's actually a file that lives there, and I don't see why the global config would want to set this.

We should also say what this is for: "Relative paths mentioned the fragment are resolved against this directory."

95

Paths in a fragment are conventionally Posix paths, so this dir should be one too before we can use string prefixes. It's also more convenient if it has a trailing slash (current code has a bug where /foo matches /foo2).

This could be handled by the creator of SourceInfo, or we could accept native paths here (with/without slashes) and normalize at the start of compilation. Latter is probably slightly less error-prone.

96

Not sure "Fragment" adds anything here - Directory is a little vaguer than ideal but FragmentDirectory just seems... longer.

98

Why is this separate from FragmentDirectory?

(i.e. when would is it not FragmentDirectory.empty() ? Absolute : Relative)

clang-tools-extra/clangd/ConfigProvider.cpp

36

the associated directory is invariant, like the path - consider making it a member set at construction time instead of plumbing it through all the functions

124

this seems like a surprising place to encode this choice (why is fromYAMLFile coupled to the idea that it's a global config file not associated with any dir?)

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

122

can we use a slightly less degenerate example to verify that we're applying the regex to the correct string?

123

please use testPath() to generate fake paths.

I think this would have caught path-handling bugs on windows.

146

can we fold this into the test above? it seems pretty duplicative.

[kadircet]

• Drop PathSpec, deduce it from emptyness of FragmentDirectory.
• Make tests more homogenous.
• Add path cannonicalization to compile stage.

clang-tools-extra/clangd/ConfigProvider.cpp

124

It was mostly because the only user of fromYAMLFile was user-config provider. Would you rather have this as an additional parameter instead?

I can't see any other way of updating that information directly from the user, apart from changing the interface of Provider to have some sort of setPathSpec method.

[sammccall]

clang-tools-extra/clangd/ConfigCompile.cpp

73

nit: associated with again (or just "normalized Fragment::SourceInfo::Directory")

153

maybe pull out a method Optional<StringRef> configRelative(StringRef)?
This would avoid the duplication

159

not needed as you've ensured FragmentDir ends with a slash

clang-tools-extra/clangd/ConfigProvider.cpp

70

nit: Directory matching Fragment.Source.Directory?
(the abbreviation isn't terrible but consistency is nice)

124

Yes, I'd prefer this to be a parameter (I suppose the dir seems slightly cleaner than a boolean).
The fact that user-config is the only user seems like a coincidence, I'd rather express the decision at that level.

(On the other hand, the fact that RelFileProvider *does* set Dir isn't a coincidence, it seems OK to hardcode that)

[kadircet]

• Make Directory a parameter of fromYAMLFile.
• Factor out configRelative logic to a helper.

[kadircet]

clang-tools-extra/clangd/ConfigCompile.cpp

153

Went with just StringRef as otuput and made empty imply a missmatch (i.e. being out-of-tree)

[sammccall]

clang-tools-extra/clangd/ConfigCompile.cpp

48

not that if Path == FragmentDir you're going to return "" which means "not under".
I don't think this can happen yet, but it's a trap for the future...

You might want the last line to be "return FragmentDir.empty() ? "." : Path".

[kadircet]

• Return "." if Path == FragmentDir in conifgRelative.

clang-tools-extra/clangd/ConfigCompile.cpp

48

i think you meant Path.empty() ? "." : Path, right?

[sammccall]

Still LG but a few more nits

clang-tools-extra/clangd/ConfigCompile.cpp

43

you should document+verify what this does if FragmentDir is empty, since we are triggering that case

48

oops, no I think I meant (Path.empty() && !FragmentDir.empty() ? "." : Path.
You don't want configRelative("","") to be ".".

So either that or handle the FragmentDir-is-empty case upfront.

150

nit: why the rename rather than just capturing?

[kadircet]

clang-tools-extra/clangd/ConfigCompile.cpp

150

because it is actually a member of FragmentCompiler and not a scoped variable :/

[kadircet]

• Explicitly handle FragmentDir.empty() case by returning Path as-is.

clang-tools-extra/clangd/ConfigCompile.cpp

 #include "llvm/Support/SMLoc.h"
 #include "llvm/Support/SourceMgr.h"
 
 namespace clang {
 namespace clangd {
 namespace config {
 namespace {
 
 struct CompiledFragmentImpl {
   // The independent conditions to check before using settings from this config.
   // The following fragment has *two* conditions:
   //   If: { Platform: [mac, linux], PathMatch: foo/.* }
   // All of them must be satisfied: the platform and path conditions are ANDed.
   // The OR logic for the platform condition is implemented inside the function.
   std::vector<llvm::unique_function<bool(const Params &) const>> Conditions;
   // Mutations that this fragment will apply to the configuration.
   // These are invoked only if the conditions are satisfied.
   std::vector<llvm::unique_function<void(Config &) const>> Apply;
 
   bool operator()(const Params &P, Config &C) const {
     for (const auto &C : Conditions) {
       if (!C(P)) {
         dlog("Config fragment {0}: condition not met", this);
         return false;
       }
     }
     dlog("Config fragment {0}: applying {1} rules", this, Apply.size());
     for (const auto &A : Apply)
       A(C);
     return true;
   }
 };
 
 // Wrapper around condition compile() functions to reduce arg-passing.
 struct FragmentCompiler {
   CompiledFragmentImpl &Out;
   DiagnosticCallback Diagnostic;
   llvm::SourceMgr *SourceMgr;
+  Fragment::SourceInfo &FragmentSource;
 
   llvm::Optional<llvm::Regex> compileRegex(const Located<std::string> &Text) {
     std::string Anchored = "^(" + *Text + ")$";
     llvm::Regex Result(Anchored);
     std::string RegexError;
     if (!Result.isValid(RegexError)) {
       diag(Error, "Invalid regex " + Anchored + ": " + RegexError, Text.Range);
       return llvm::None;
void compile(Fragment::IfBlock &&F) {
 
     auto PathMatch = std::make_unique<std::vector<llvm::Regex>>();
     for (auto &Entry : F.PathMatch) {
       if (auto RE = compileRegex(Entry))
         PathMatch->push_back(std::move(*RE));
     }
     if (!PathMatch->empty()) {
       Out.Conditions.push_back(
-          [PathMatch(std::move(PathMatch))](const Params &P) {
+          [PathMatch(std::move(PathMatch)), PathSpec(FragmentSource.PathSpec),
+           FragmentDir(FragmentSource.FragmentDirectory)](const Params &P) {
             if (P.Path.empty())
               return false;
+            llvm::StringRef Path = P.Path;
+            // In relative mode ignore paths rooted outside FragmentDirectory
+            // and strip the prefix for relative matching.
+            if (PathSpec == Fragment::SourceInfo::Relative &&
+                !Path.consume_front(FragmentDir)) {
+              return false;
+            }
             return llvm::any_of(*PathMatch, [&](const llvm::Regex &RE) {
-              return RE.match(P.Path);
+              return RE.match(Path);
             });
           });
     }
 
     auto PathExclude = std::make_unique<std::vector<llvm::Regex>>();
     for (auto &Entry : F.PathExclude) {
       if (auto RE = compileRegex(Entry))
         PathExclude->push_back(std::move(*RE));
     }
     if (!PathExclude->empty()) {
       Out.Conditions.push_back(
-          [PathExclude(std::move(PathExclude))](const Params &P) {
+          [PathExclude(std::move(PathExclude)),
+           PathSpec(FragmentSource.PathSpec),
+           FragmentDir(FragmentSource.FragmentDirectory)](const Params &P) {
             if (P.Path.empty())
               return false;
+            llvm::StringRef Path = P.Path;
+            // In relative mode ignore paths rooted outside FragmentDirectory
+            // and strip the prefix for relative matching.
+            if (PathSpec == Fragment::SourceInfo::Relative) {
+              if (!Path.consume_front(FragmentDir))
+                return true;
+              Path.consume_front("/");
+            }
             return llvm::none_of(*PathExclude, [&](const llvm::Regex &RE) {
-              return RE.match(P.Path);
+              return RE.match(Path);
             });
           });
     }
   }
 
   void compile(Fragment::CompileFlagsBlock &&F) {
     if (!F.Remove.empty()) {
       auto Remove = std::make_shared<ArgStripper>();
if (auto *SM = Source.Manager.get()) {
     ConfigFile = SM->getBufferInfo(BufID).Buffer->getBufferIdentifier();
   }
   trace::Span Tracer("ConfigCompile");
   SPAN_ATTACH(Tracer, "ConfigFile", ConfigFile);
   auto Result = std::make_shared<CompiledFragmentImpl>();
   vlog("Config fragment: compiling {0}:{1} -> {2}", ConfigFile, LineCol.first,
        Result.get());
 
-  FragmentCompiler{*Result, D, Source.Manager.get()}.compile(std::move(*this));
+  FragmentCompiler{*Result, D, Source.Manager.get(), Source}.compile(
+      std::move(*this));
   // Return as cheaply-copyable wrapper.
   return [Result(std::move(Result))](const Params &P, Config &C) {
     return (*Result)(P, C);
   };
 }
 
 } // namespace config
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/ConfigFragment.h

struct SourceInfo {
     /// Retains a buffer of the original source this fragment was parsed from.
     /// Locations within Located<T> objects point into this SourceMgr.
     /// Shared because multiple fragments are often parsed from one (YAML) file.
     /// May be null, then all locations should be ignored.
     std::shared_ptr<llvm::SourceMgr> Manager;
     /// The start of the original source for this fragment.
     /// Only valid if SourceManager is set.
     llvm::SMLoc Location;
+    /// Absolute path to directory containing the fragment. Doesn't contain the
+    /// trailing slash.
+    std::string FragmentDirectory;
+    /// Describes how paths specified in this fragment should be treated.
+    enum PathSpecification {
+      Absolute,
+      Relative,
+    } PathSpec = Absolute;
   };
   SourceInfo Source;
 
   /// Conditions in the If block restrict when a Fragment applies.
   ///
   /// Each separate condition must match (combined with AND).
   /// When one condition has multiple values, any may match (combined with OR).
   /// e.g. `PathMatch: [foo/.*, bar/.*]` matches files in either directory.

clang-tools-extra/clangd/ConfigProvider.cpp

 //===--- ConfigProvider.cpp - Loading of user configuration ---------------===//
 //
 // 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 "ConfigProvider.h"
 #include "Config.h"
 #include "ConfigFragment.h"
 #include "support/ThreadsafeFS.h"
 #include "support/Trace.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Path.h"
 #include <chrono>
 #include <mutex>
 
 namespace clang {
 namespace clangd {
 namespace config {
 
 // Threadsafe cache around reading a YAML config file from disk.
 class FileConfigCache {
   std::mutex Mu;
   std::chrono::steady_clock::time_point ValidTime = {};
   llvm::SmallVector<CompiledFragment, 1> CachedValue;
   llvm::sys::TimePoint<> MTime = {};
   unsigned Size = -1;
 
   // Called once we are sure we want to read the file.
   // REQUIRES: Cache keys are set. Mutex must be held.
-  void fillCacheFromDisk(llvm::vfs::FileSystem &FS, DiagnosticCallback DC) {
+  void fillCacheFromDisk(llvm::vfs::FileSystem &FS, DiagnosticCallback DC,
+                         Fragment::SourceInfo::PathSpecification PathSpec) {
     CachedValue.clear();
 
     auto Buf = FS.getBufferForFile(Path);
     // If we failed to read (but stat succeeded), don't cache failure.
     if (!Buf) {
       Size = -1;
       MTime = {};
       return;
     }
 
     // If file changed between stat and open, we don't know its mtime.
     // For simplicity, don't cache the value in this case (use a bad key).
     if (Buf->get()->getBufferSize() != Size) {
       Size = -1;
       MTime = {};
     }
 
+    llvm::StringRef FragmentDir = llvm::sys::path::parent_path(Path);
+    FragmentDir.consume_back("/");
     // Finally parse and compile the actual fragments.
     for (auto &Fragment :
-         Fragment::parseYAML(Buf->get()->getBuffer(), Path, DC))
+         Fragment::parseYAML(Buf->get()->getBuffer(), Path, DC)) {
+      Fragment.Source.FragmentDirectory = FragmentDir.str();
+      Fragment.Source.PathSpec = PathSpec;
       CachedValue.push_back(std::move(Fragment).compile(DC));
     }
+  }
 
 public:
   // Must be set before the cache is used. Not a constructor param to allow
   // computing ancestor-relative paths to be deferred.
   std::string Path;
 
   // Retrieves up-to-date config fragments from disk.
   // A cached result may be reused if the mtime and size are unchanged.
   // (But several concurrent read()s can miss the cache after a single change).
   // Future performance ideas:
   // - allow caches to be reused based on short elapsed walltime
   // - allow latency-sensitive operations to skip revalidating the cache
   void read(const ThreadsafeFS &TFS, DiagnosticCallback DC,
             llvm::Optional<std::chrono::steady_clock::time_point> FreshTime,
+            Fragment::SourceInfo::PathSpecification PathSpec,
             std::vector<CompiledFragment> &Out) {
     std::lock_guard<std::mutex> Lock(Mu);
     // We're going to update the cache and return whatever's in it.
     auto Return = llvm::make_scope_exit(
         [&] { llvm::copy(CachedValue, std::back_inserter(Out)); });
 
     // Return any sufficiently recent result without doing any further work.
     if (FreshTime && ValidTime >= FreshTime)
void read(const ThreadsafeFS &TFS, DiagnosticCallback DC,
     }
     // If the modified-time and size match, assume the content does too.
     if (Size == Stat->getSize() && MTime == Stat->getLastModificationTime())
       return;
 
     // OK, the file has actually changed. Update cache key, compute new value.
     Size = Stat->getSize();
     MTime = Stat->getLastModificationTime();
-    fillCacheFromDisk(*FS, DC);
+    fillCacheFromDisk(*FS, DC, PathSpec);
   }
 };
 
 std::unique_ptr<Provider> Provider::fromYAMLFile(llvm::StringRef AbsPath,
                                                  const ThreadsafeFS &FS) {
   class AbsFileProvider : public Provider {
     mutable FileConfigCache Cache; // threadsafe
     const ThreadsafeFS &FS;
 
     std::vector<CompiledFragment>
     getFragments(const Params &P, DiagnosticCallback DC) const override {
       std::vector<CompiledFragment> Result;
-      Cache.read(FS, DC, P.FreshTime, Result);
+      Cache.read(FS, DC, P.FreshTime, Fragment::SourceInfo::Absolute, Result);
       return Result;
     };
 
   public:
     AbsFileProvider(llvm::StringRef Path, const ThreadsafeFS &FS) : FS(FS) {
       assert(llvm::sys::path::is_absolute(Path));
       Cache.Path = Path.str();
     }
getFragments(const Params &P, DiagnosticCallback DC) const override {
             R.first->second.Path = ConfigPath.str().str();
           }
           Caches.push_back(&R.first->second);
         }
       }
       // Finally query each individual file.
       // This will take a (per-file) lock for each file that actually exists.
       std::vector<CompiledFragment> Result;
-      for (FileConfigCache *Cache : Caches)
-        Cache->read(FS, DC, P.FreshTime, Result);
+      for (FileConfigCache *Cache : Caches) {
+        Cache->read(FS, DC, P.FreshTime, Fragment::SourceInfo::Relative,
+                    Result);
+      }
       return Result;
     };
 
   public:
     RelFileProvider(llvm::StringRef RelPath, const ThreadsafeFS &FS)
         : RelPath(RelPath), FS(FS) {
       assert(llvm::sys::path::is_relative(RelPath));
     }

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

TEST_F(ConfigCompileTests, Index) {
   EXPECT_EQ(Conf.Index.Background, Config::BackgroundPolicy::Build)
       << "by default";
   EXPECT_THAT(
       Diags.Diagnostics,
       ElementsAre(DiagMessage(
           "Invalid Background value 'Foo'. Valid values are Build, Skip.")));
 }
 
+TEST_F(ConfigCompileTests, PathSpecMatch) {
+  const Fragment BaseFrag = [] {
+    Fragment Frag;
+    Frag.If.PathMatch.emplace_back(".*");
+    Frag.Source.FragmentDirectory = "/baz";
+    return Frag;
+  }();
+
+  Parm.Path = "/foo/bar.h";
+  // * matches everything with absolute spec.
+  Frag = BaseFrag;
+  Frag.Source.PathSpec = Fragment::SourceInfo::Absolute;
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_TRUE(compileAndApply());
+  // Relative should fail to match as /foo/bar.h doesn't reside under /baz/.
+  Frag = BaseFrag;
+  Frag.Source.PathSpec = Fragment::SourceInfo::Relative;
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_FALSE(compileAndApply());
+  // Relative should pass now.
+  Frag = BaseFrag;
+  Parm.Path = "/baz/anything";
+  Frag.Source.PathSpec = Fragment::SourceInfo::Relative;
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_TRUE(compileAndApply());
+}
+
+TEST_F(ConfigCompileTests, PathSpecExclude) {
+  const Fragment BaseFrag = [] {
+    Fragment Frag;
+    Frag.If.PathExclude.emplace_back(".*");
+    Frag.Source.FragmentDirectory = "/baz";
+    return Frag;
+  }();
+
+  Parm.Path = "/foo/bar.h";
+  // * matches everything with absolute spec.
+  Frag = BaseFrag;
+  Frag.Source.PathSpec = Fragment::SourceInfo::Absolute;
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_FALSE(compileAndApply());
+  // Relative should fail to match as /foo/bar.h doesn't reside under /baz/.
+  Frag = BaseFrag;
+  Frag.Source.PathSpec = Fragment::SourceInfo::Relative;
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_TRUE(compileAndApply());
+  // Relative should pass now.
+  Frag = BaseFrag;
+  Parm.Path = "/baz/anything";
+  Frag.Source.PathSpec = Fragment::SourceInfo::Relative;
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_FALSE(compileAndApply());
+}
 } // namespace
 } // namespace config
 } // namespace clangd
 } // namespace clang

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

 //===-- ConfigProviderTests.cpp -------------------------------------------===//
 //
 // 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 "Config.h"
 #include "ConfigProvider.h"
 #include "ConfigTesting.h"
 #include "TestFS.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/SourceMgr.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <atomic>
 #include <chrono>
 
 namespace clang {
 namespace clangd {
TEST(ProviderTest, Staleness) {
 
   // Cache revalidated by (default) policy.
   FS.Files.erase("foo.yaml");
   Cfg = P->getConfig(Params(), Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty());
   EXPECT_THAT(getAddedArgs(Cfg), IsEmpty());
 }
 
+TEST(ProviderTest, SourceInfo) {
+  MockFS FS;
+
+  FS.Files["baz/foo.yaml"] = R"yaml(
+If:
+  PathMatch: .*
+  PathExclude: bar.h
+CompileFlags:
+  Add: bar
+)yaml";
+  const auto BarPath = testPath("baz/bar.h", llvm::sys::path::Style::posix);
+  CapturedDiags Diags;
+  Params Bar;
+  Bar.Path = BarPath;
+
+  // This should be an absolute match/exclude hence baz/bar.h should not be
+  // excluded.
+  auto P = Provider::fromYAMLFile(testPath("baz/foo.yaml"), FS);
+  auto Cfg = P->getConfig(Bar, Diags.callback());
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("bar"));
+  Diags.Diagnostics.clear();
+
+  // This should be a relative match/exclude hence baz/bar.h should be excluded.
+  P = Provider::fromAncestorRelativeYAMLFiles("foo.yaml", FS);
+  Cfg = P->getConfig(Bar, Diags.callback());
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_THAT(getAddedArgs(Cfg), IsEmpty());
+  Diags.Diagnostics.clear();
+}
 } // namespace
 } // namespace config
 } // namespace clangd
 } // namespace clang