sammccall created this revision.
sammccall added a reviewer: hokein.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous,
MaskRay, ilya-biryukov.
Herald added a project: clang.
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
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D83436
Files:
clang-tools-extra/clangd/ConfigYAML.cpp
clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -98,10 +98,25 @@
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
Index: clang-tools-extra/clangd/ConfigYAML.cpp
===================================================================
--- clang-tools-extra/clangd/ConfigYAML.cpp
+++ clang-tools-extra/clangd/ConfigYAML.cpp
@@ -26,6 +26,7 @@
class Parser {
llvm::SourceMgr &SM;
+ bool HadError = false;
public:
Parser(llvm::SourceMgr &SM) : SM(SM) {}
@@ -35,40 +36,38 @@
// The private parse() helpers follow the same pattern.
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 +78,7 @@
// 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 +93,17 @@
}
// 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 +113,12 @@
}
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 +128,6 @@
Unknown(**Key);
}
}
- return true;
}
};
@@ -154,6 +152,7 @@
} 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 +166,7 @@
// 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 +196,7 @@
&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;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
