[clang-tools-extra] 7f059a2 - [clangd] Handle absolute/relative path specifications in Config
Kadir Cetinkaya via cfe-commits
cfe-commits at lists.llvm.org
Tue Nov 3 12:46:02 PST 2020
Author: Kadir Cetinkaya
Date: 2020-11-03T21:45:35+01:00
New Revision: 7f059a258a1dbfc240a8d526b5d23d238a3d84f7
URL: https://github.com/llvm/llvm-project/commit/7f059a258a1dbfc240a8d526b5d23d238a3d84f7
DIFF: https://github.com/llvm/llvm-project/commit/7f059a258a1dbfc240a8d526b5d23d238a3d84f7.diff
LOG: [clangd] Handle absolute/relative path specifications in Config
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.
Differential Revision: https://reviews.llvm.org/D90270
Added:
Modified:
clang-tools-extra/clangd/ConfigCompile.cpp
clang-tools-extra/clangd/ConfigFragment.h
clang-tools-extra/clangd/ConfigProvider.cpp
clang-tools-extra/clangd/ConfigProvider.h
clang-tools-extra/clangd/tool/ClangdMain.cpp
clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/ConfigCompile.cpp b/clang-tools-extra/clangd/ConfigCompile.cpp
index 62fda5c9eacc..f73db45d3ab5 100644
--- a/clang-tools-extra/clangd/ConfigCompile.cpp
+++ b/clang-tools-extra/clangd/ConfigCompile.cpp
@@ -26,20 +26,34 @@
#include "CompileCommands.h"
#include "Config.h"
#include "ConfigFragment.h"
+#include "ConfigProvider.h"
#include "support/Logger.h"
#include "support/Trace.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/StringSwitch.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/Regex.h"
#include "llvm/Support/SMLoc.h"
#include "llvm/Support/SourceMgr.h"
+#include <string>
namespace clang {
namespace clangd {
namespace config {
namespace {
+// Returns an empty stringref if Path is not under FragmentDir. Returns Path
+// as-is when FragmentDir is empty.
+llvm::StringRef configRelative(llvm::StringRef Path,
+ llvm::StringRef FragmentDir) {
+ if (FragmentDir.empty())
+ return Path;
+ if (!Path.consume_front(FragmentDir))
+ return llvm::StringRef();
+ return Path.empty() ? "." : Path;
+}
+
struct CompiledFragmentImpl {
// The independent conditions to check before using settings from this config.
// The following fragment has *two* conditions:
@@ -67,9 +81,14 @@ struct CompiledFragmentImpl {
// Wrapper around condition compile() functions to reduce arg-passing.
struct FragmentCompiler {
+ 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) {
std::string Anchored = "^(" + *Text + ")$";
@@ -129,6 +148,11 @@ struct FragmentCompiler {
}
void compile(Fragment &&F) {
+ if (!F.Source.Directory.empty()) {
+ FragmentDirectory = llvm::sys::path::convert_to_slash(F.Source.Directory);
+ if (FragmentDirectory.back() != '/')
+ FragmentDirectory += '/';
+ }
compile(std::move(F.If));
compile(std::move(F.CompileFlags));
compile(std::move(F.Index));
@@ -145,11 +169,16 @@ struct FragmentCompiler {
}
if (!PathMatch->empty()) {
Out.Conditions.push_back(
- [PathMatch(std::move(PathMatch))](const Params &P) {
+ [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(P.Path);
+ return RE.match(Path);
});
});
}
@@ -161,11 +190,16 @@ struct FragmentCompiler {
}
if (!PathExclude->empty()) {
Out.Conditions.push_back(
- [PathExclude(std::move(PathExclude))](const Params &P) {
+ [PathExclude(std::move(PathExclude)),
+ 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 true;
return llvm::none_of(*PathExclude, [&](const llvm::Regex &RE) {
- return RE.match(P.Path);
+ return RE.match(Path);
});
});
}
diff --git a/clang-tools-extra/clangd/ConfigFragment.h b/clang-tools-extra/clangd/ConfigFragment.h
index 5b4865e48f3a..7e07e9d065b3 100644
--- a/clang-tools-extra/clangd/ConfigFragment.h
+++ b/clang-tools-extra/clangd/ConfigFragment.h
@@ -91,6 +91,9 @@ struct Fragment {
/// The start of the original source for this fragment.
/// Only valid if SourceManager is set.
llvm::SMLoc Location;
+ /// Absolute path to directory the fragment is associated with. Relative
+ /// paths mentioned in the fragment are resolved against this.
+ std::string Directory;
};
SourceInfo Source;
diff --git a/clang-tools-extra/clangd/ConfigProvider.cpp b/clang-tools-extra/clangd/ConfigProvider.cpp
index a56cdd755322..5a00ec5048c0 100644
--- a/clang-tools-extra/clangd/ConfigProvider.cpp
+++ b/clang-tools-extra/clangd/ConfigProvider.cpp
@@ -13,9 +13,11 @@
#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>
+#include <string>
namespace clang {
namespace clangd {
@@ -51,14 +53,18 @@ class FileConfigCache {
// 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.Directory = Directory;
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;
+ // Directory associated with this fragment.
+ std::string Directory;
// Retrieves up-to-date config fragments from disk.
// A cached result may be reused if the mtime and size are unchanged.
@@ -105,6 +111,7 @@ class FileConfigCache {
};
std::unique_ptr<Provider> Provider::fromYAMLFile(llvm::StringRef AbsPath,
+ llvm::StringRef Directory,
const ThreadsafeFS &FS) {
class AbsFileProvider : public Provider {
mutable FileConfigCache Cache; // threadsafe
@@ -118,13 +125,16 @@ std::unique_ptr<Provider> Provider::fromYAMLFile(llvm::StringRef AbsPath,
};
public:
- AbsFileProvider(llvm::StringRef Path, const ThreadsafeFS &FS) : FS(FS) {
+ AbsFileProvider(llvm::StringRef Path, llvm::StringRef Directory,
+ const ThreadsafeFS &FS)
+ : FS(FS) {
assert(llvm::sys::path::is_absolute(Path));
Cache.Path = Path.str();
+ Cache.Directory = Directory.str();
}
};
- return std::make_unique<AbsFileProvider>(AbsPath, FS);
+ return std::make_unique<AbsFileProvider>(AbsPath, Directory, FS);
}
std::unique_ptr<Provider>
@@ -170,6 +180,7 @@ Provider::fromAncestorRelativeYAMLFiles(llvm::StringRef RelPath,
llvm::SmallString<256> ConfigPath = Ancestor;
path::append(ConfigPath, RelPath);
R.first->second.Path = ConfigPath.str().str();
+ R.first->second.Directory = Ancestor.str();
}
Caches.push_back(&R.first->second);
}
@@ -177,8 +188,9 @@ Provider::fromAncestorRelativeYAMLFiles(llvm::StringRef RelPath,
// 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)
+ for (FileConfigCache *Cache : Caches) {
Cache->read(FS, DC, P.FreshTime, Result);
+ }
return Result;
};
diff --git a/clang-tools-extra/clangd/ConfigProvider.h b/clang-tools-extra/clangd/ConfigProvider.h
index 1ef33c79c1e8..54f4b1467236 100644
--- a/clang-tools-extra/clangd/ConfigProvider.h
+++ b/clang-tools-extra/clangd/ConfigProvider.h
@@ -18,6 +18,7 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIGPROVIDER_H
#include "llvm/ADT/FunctionExtras.h"
+#include "llvm/ADT/StringRef.h"
#include "llvm/Support/SMLoc.h"
#include "llvm/Support/SourceMgr.h"
#include <chrono>
@@ -61,8 +62,10 @@ class Provider {
public:
virtual ~Provider() = default;
- // Reads fragments from a single YAML file with a fixed path.
- static std::unique_ptr<Provider> fromYAMLFile(llvm::StringRef AbsPathPath,
+ /// Reads fragments from a single YAML file with a fixed path. If non-empty,
+ /// Directory will be used to resolve relative paths in the fragments.
+ static std::unique_ptr<Provider> fromYAMLFile(llvm::StringRef AbsPath,
+ llvm::StringRef Directory,
const ThreadsafeFS &);
// Reads fragments from YAML files found relative to ancestors of Params.Path.
//
diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp
index faa8c7508a93..ae5b16ebce7f 100644
--- a/clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -790,7 +790,8 @@ clangd accepts flags on the commandline, and in the CLANGD_FLAGS environment var
if (llvm::sys::path::user_config_directory(UserConfig)) {
llvm::sys::path::append(UserConfig, "clangd", "config.yaml");
vlog("User config file is {0}", UserConfig);
- ProviderStack.push_back(config::Provider::fromYAMLFile(UserConfig, TFS));
+ ProviderStack.push_back(
+ config::Provider::fromYAMLFile(UserConfig, /*Directory=*/"", TFS));
} else {
elog("Couldn't determine user config file, not loading");
}
diff --git a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
index c8465dc70edb..f791820f7e45 100644
--- a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -9,8 +9,12 @@
#include "Config.h"
#include "ConfigFragment.h"
#include "ConfigTesting.h"
+#include "TestFS.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Path.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
+#include <string>
namespace clang {
namespace clangd {
@@ -116,6 +120,62 @@ TEST_F(ConfigCompileTests, Index) {
"Invalid Background value 'Foo'. Valid values are Build, Skip.")));
}
+TEST_F(ConfigCompileTests, PathSpecMatch) {
+ auto BarPath = llvm::sys::path::convert_to_slash(testPath("foo/bar.h"));
+ Parm.Path = BarPath;
+
+ struct {
+ std::string Directory;
+ std::string PathSpec;
+ bool ShouldMatch;
+ } Cases[] = {
+ {
+ // Absolute path matches.
+ "",
+ llvm::sys::path::convert_to_slash(testPath("foo/bar.h")),
+ true,
+ },
+ {
+ // Absolute path fails.
+ "",
+ llvm::sys::path::convert_to_slash(testPath("bar/bar.h")),
+ false,
+ },
+ {
+ // Relative should fail to match as /foo/bar.h doesn't reside under
+ // /baz/.
+ testPath("baz"),
+ "bar\\.h",
+ false,
+ },
+ {
+ // Relative should pass with /foo as directory.
+ testPath("foo"),
+ "bar\\.h",
+ true,
+ },
+ };
+
+ // PathMatch
+ for (const auto &Case : Cases) {
+ Frag = {};
+ Frag.If.PathMatch.emplace_back(Case.PathSpec);
+ Frag.Source.Directory = Case.Directory;
+ EXPECT_EQ(compileAndApply(), Case.ShouldMatch);
+ ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+ }
+
+ // PathEclude
+ for (const auto &Case : Cases) {
+ SCOPED_TRACE(Case.Directory);
+ SCOPED_TRACE(Case.PathSpec);
+ Frag = {};
+ Frag.If.PathExclude.emplace_back(Case.PathSpec);
+ Frag.Source.Directory = Case.Directory;
+ EXPECT_NE(compileAndApply(), Case.ShouldMatch);
+ ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+ }
+}
} // namespace
} // namespace config
} // namespace clangd
diff --git a/clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp b/clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
index 0cf582410ff8..29ab8f3b3606 100644
--- a/clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
@@ -10,6 +10,7 @@
#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"
@@ -91,7 +92,7 @@ TEST(ProviderTest, FromYAMLFile) {
FS.Files["foo.yaml"] = AddFooWithErr;
CapturedDiags Diags;
- auto P = Provider::fromYAMLFile(testPath("foo.yaml"), FS);
+ auto P = Provider::fromYAMLFile(testPath("foo.yaml"), /*Directory=*/"", FS);
auto Cfg = P->getConfig(Params(), Diags.callback());
EXPECT_THAT(Diags.Diagnostics,
ElementsAre(DiagMessage("Unknown CompileFlags key Unknown")));
@@ -159,7 +160,7 @@ TEST(ProviderTest, Staleness) {
Params MustBeFresh;
MustBeFresh.FreshTime = StartTime + std::chrono::hours(1);
CapturedDiags Diags;
- auto P = Provider::fromYAMLFile(testPath("foo.yaml"), FS);
+ auto P = Provider::fromYAMLFile(testPath("foo.yaml"), /*Directory=*/"", FS);
// Initial query always reads, regardless of policy.
FS.Files["foo.yaml"] = AddFooWithErr;
@@ -187,6 +188,37 @@ TEST(ProviderTest, Staleness) {
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"), /*Directory=*/"", 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
More information about the cfe-commits
mailing list