[clang-tools-extra] f365186 - [clangd] Fix error handling in config.yaml parsing.
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 9 01:20:27 PDT 2020
Author: Sam McCall
Date: 2020-07-09T10:20:18+02:00
New Revision: f36518637d7dfe5f8e619db1bd65dc90c92b5afa
URL: https://github.com/llvm/llvm-project/commit/f36518637d7dfe5f8e619db1bd65dc90c92b5afa
DIFF: https://github.com/llvm/llvm-project/commit/f36518637d7dfe5f8e619db1bd65dc90c92b5afa.diff
LOG: [clangd] Fix error handling in config.yaml parsing.
Summary:
A few things were broken:
- use of Document::parseBlockNode() is incorrect and prevents moving to the
next doc in error cases. Use getRoot() instead.
- bailing out in the middle of iterating over a list/dict isn't allowed,
unless you are going to throw away the parser: the next skip() asserts.
Always consume all items.
- There were two concepts of fatal errors: error-diagnostics and drop-fragment.
(The latter is the "return false" case in the parser). They didn't coincide.
Now, parser errors and explicitly emitted error diagnostics are fatal.
Fixes https://github.com/clangd/clangd/issues/452
Reviewers: hokein
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D83436
Added:
Modified:
clang-tools-extra/clangd/ConfigYAML.cpp
clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/ConfigYAML.cpp b/clang-tools-extra/clangd/ConfigYAML.cpp
index 1201c9a5d523..0674c6030903 100644
--- a/clang-tools-extra/clangd/ConfigYAML.cpp
+++ b/clang-tools-extra/clangd/ConfigYAML.cpp
@@ -26,49 +26,47 @@ using llvm::yaml::SequenceNode;
class Parser {
llvm::SourceMgr &SM;
+ bool HadError = false;
public:
Parser(llvm::SourceMgr &SM) : SM(SM) {}
// Tries to parse N into F, returning false if it failed and we couldn't
- // meaningfully recover (e.g. YAML syntax error broke the stream).
- // The private parse() helpers follow the same pattern.
+ // meaningfully recover (YAML syntax error, or hard semantic error).
bool parse(Fragment &F, Node &N) {
DictParser Dict("Config", this);
- Dict.handle("If", [&](Node &N) { return parse(F.If, N); });
- Dict.handle("CompileFlags",
- [&](Node &N) { return parse(F.CompileFlags, N); });
- return Dict.parse(N);
+ Dict.handle("If", [&](Node &N) { parse(F.If, N); });
+ Dict.handle("CompileFlags", [&](Node &N) { parse(F.CompileFlags, N); });
+ Dict.parse(N);
+ return !(N.failed() || HadError);
}
private:
- bool parse(Fragment::IfBlock &F, Node &N) {
+ void parse(Fragment::IfBlock &F, Node &N) {
DictParser Dict("If", this);
Dict.unrecognized(
[&](llvm::StringRef) { F.HasUnrecognizedCondition = true; });
Dict.handle("PathMatch", [&](Node &N) {
if (auto Values = scalarValues(N))
F.PathMatch = std::move(*Values);
- return !N.failed();
});
- return Dict.parse(N);
+ Dict.parse(N);
}
- bool parse(Fragment::CompileFlagsBlock &F, Node &N) {
+ void parse(Fragment::CompileFlagsBlock &F, Node &N) {
DictParser Dict("CompileFlags", this);
Dict.handle("Add", [&](Node &N) {
if (auto Values = scalarValues(N))
F.Add = std::move(*Values);
- return !N.failed();
});
- return Dict.parse(N);
+ Dict.parse(N);
}
// Helper for parsing mapping nodes (dictionaries).
// We don't use YamlIO as we want to control over unknown keys.
class DictParser {
llvm::StringRef Description;
- std::vector<std::pair<llvm::StringRef, std::function<bool(Node &)>>> Keys;
+ std::vector<std::pair<llvm::StringRef, std::function<void(Node &)>>> Keys;
std::function<void(llvm::StringRef)> Unknown;
Parser *Outer;
@@ -79,7 +77,7 @@ class Parser {
// Parse is called when Key is encountered, and passed the associated value.
// It should emit diagnostics if the value is invalid (e.g. wrong type).
// If Key is seen twice, Parse runs only once and an error is reported.
- void handle(llvm::StringLiteral Key, std::function<bool(Node &)> Parse) {
+ void handle(llvm::StringLiteral Key, std::function<void(Node &)> Parse) {
for (const auto &Entry : Keys) {
(void) Entry;
assert(Entry.first != Key && "duplicate key handler");
@@ -94,16 +92,17 @@ class Parser {
}
// Process a mapping node and call handlers for each key/value pair.
- bool parse(Node &N) const {
+ void parse(Node &N) const {
if (N.getType() != Node::NK_Mapping) {
Outer->error(Description + " should be a dictionary", N);
- return false;
+ return;
}
llvm::SmallSet<std::string, 8> Seen;
+ // We *must* consume all items, even on error, or the parser will assert.
for (auto &KV : llvm::cast<MappingNode>(N)) {
auto *K = KV.getKey();
if (!K) // YAMLParser emitted an error.
- return false;
+ continue;
auto Key = Outer->scalarValue(*K, "Dictionary key");
if (!Key)
continue;
@@ -113,13 +112,12 @@ class Parser {
}
auto *Value = KV.getValue();
if (!Value) // YAMLParser emitted an error.
- return false;
+ continue;
bool Matched = false;
for (const auto &Handler : Keys) {
if (Handler.first == **Key) {
- if (!Handler.second(*Value))
- return false;
Matched = true;
+ Handler.second(*Value);
break;
}
}
@@ -129,7 +127,6 @@ class Parser {
Unknown(**Key);
}
}
- return true;
}
};
@@ -154,6 +151,7 @@ class Parser {
} else if (auto *S = llvm::dyn_cast<BlockScalarNode>(&N)) {
Result.emplace_back(S->getValue().str(), N.getSourceRange());
} else if (auto *S = llvm::dyn_cast<SequenceNode>(&N)) {
+ // We *must* consume all items, even on error, or the parser will assert.
for (auto &Child : *S) {
if (auto Value = scalarValue(Child, "List item"))
Result.push_back(std::move(*Value));
@@ -167,6 +165,7 @@ class Parser {
// Report a "hard" error, reflecting a config file that can never be valid.
void error(const llvm::Twine &Msg, const Node &N) {
+ HadError = true;
SM.PrintMessage(N.getSourceRange().Start, llvm::SourceMgr::DK_Error, Msg,
N.getSourceRange());
}
@@ -196,7 +195,7 @@ std::vector<Fragment> Fragment::parseYAML(llvm::StringRef YAML,
&Diags);
std::vector<Fragment> Result;
for (auto &Doc : llvm::yaml::Stream(*Buf, *SM)) {
- if (Node *N = Doc.parseBlockNode()) {
+ if (Node *N = Doc.getRoot()) {
Fragment Fragment;
Fragment.Source.Manager = SM;
Fragment.Source.Location = N->getSourceRange().Start;
diff --git a/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp b/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
index 3ad8b2e7c1c8..a9526ce2367c 100644
--- a/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -98,10 +98,25 @@ CompileFlags: {^
DiagKind(llvm::SourceMgr::DK_Error),
DiagPos(YAML.point()), DiagRange(llvm::None))));
- ASSERT_EQ(Results.size(), 2u);
+ ASSERT_EQ(Results.size(), 1u); // invalid fragment discarded.
EXPECT_THAT(Results.front().CompileFlags.Add, ElementsAre(Val("first")));
EXPECT_TRUE(Results.front().If.HasUnrecognizedCondition);
- EXPECT_THAT(Results.back().CompileFlags.Add, IsEmpty());
+}
+
+TEST(ParseYAML, Invalid) {
+ CapturedDiags Diags;
+ const char *YAML = R"yaml(
+If:
+
+horrible
+---
+- 1
+ )yaml";
+ auto Results = Fragment::parseYAML(YAML, "config.yaml", Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics,
+ ElementsAre(DiagMessage("If should be a dictionary"),
+ DiagMessage("Config should be a dictionary")));
+ ASSERT_THAT(Results, IsEmpty());
}
} // namespace
More information about the cfe-commits
mailing list