[PATCH] D83436: [clangd] Fix error handling in config.yaml parsing.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGf36518637d7d: [clangd] Fix error handling in config.yaml
parsing. (authored by sammccall).
Changed prior to commit:
https://reviews.llvm.org/D83436?vs=276574=276660#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83436/new/
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,49 +26,47 @@
class Parser {
llvm::SourceMgr
+ bool HadError = false;
public:
Parser(llvm::SourceMgr ) : 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 , Node ) {
DictParser Dict("Config", this);
-Dict.handle("If", [&](Node ) { return parse(F.If, N); });
-Dict.handle("CompileFlags",
-[&](Node ) { return parse(F.CompileFlags, N); });
-return Dict.parse(N);
+Dict.handle("If", [&](Node ) { parse(F.If, N); });
+Dict.handle("CompileFlags", [&](Node ) { parse(F.CompileFlags, N); });
+Dict.parse(N);
+return !(N.failed() || HadError);
}
private:
- bool parse(Fragment::IfBlock , Node ) {
+ void parse(Fragment::IfBlock , Node ) {
DictParser Dict("If", this);
Dict.unrecognized(
[&](llvm::StringRef) { F.HasUnrecognizedCondition = true; });
Dict.handle("PathMatch", [&](Node ) {
if (auto Values = scalarValues(N))
F.PathMatch = std::move(*Values);
- return !N.failed();
});
-return Dict.parse(N);
+Dict.parse(N);
}
- bool parse(Fragment::CompileFlagsBlock , Node ) {
+ void parse(Fragment::CompileFlagsBlock , Node ) {
DictParser Dict("CompileFlags", this);
Dict.handle("Add", [&](Node ) {
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>> Keys;
+std::vector>> Keys;
std::function Unknown;
Parser *Outer;
@@ -79,7 +77,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 Parse) {
+void handle(llvm::StringLiteral Key, std::function Parse) {
for (const auto : Keys) {
(void) Entry;
assert(Entry.first != Key && "duplicate key handler");
@@ -94,16 +92,17 @@
}
// Process a mapping node and call handlers for each key/value pair.
-bool parse(Node ) const {
+void parse(Node ) const {
if (N.getType() != Node::NK_Mapping) {
Outer->error(Description + " should be a dictionary", N);
-return false;
+return;
}
llvm::SmallSet Seen;
+ // We *must* consume all items, even on error, or the parser will assert.
for (auto : llvm::cast(N)) {
auto *K = KV.getKey();
if (!K) // YAMLParser emitted an error.
- return false;
+ continue;
auto Key = Outer->scalarValue(*K, "Dictionary key");
if (!Key)
[PATCH] D83436: [clangd] Fix error handling in config.yaml parsing.
hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.
thanks.
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:36
// meaningfully recover (e.g. YAML syntax error broke the stream).
// The private parse() helpers follow the same pattern.
bool parse(Fragment , Node ) {
nit: the comment is stale.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83436/new/
https://reviews.llvm.org/D83436
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D83436: [clangd] Fix error handling in config.yaml parsing.
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
+ bool HadError = false;
public:
Parser(llvm::SourceMgr ) : SM(SM) {}
@@ -35,40 +36,38 @@
// The private parse() helpers follow the same pattern.
bool parse(Fragment , Node ) {
DictParser Dict("Config", this);
-Dict.handle("If", [&](Node ) { return parse(F.If, N); });
-Dict.handle("CompileFlags",
-[&](Node ) { return parse(F.CompileFlags, N); });
-return Dict.parse(N);
+Dict.handle("If", [&](Node ) { parse(F.If, N); });
+Dict.handle("CompileFlags", [&](Node ) { parse(F.CompileFlags, N); });
+Dict.parse(N);
+return !(N.failed() || HadError);
}
private:
- bool parse(Fragment::IfBlock , Node ) {
+ void parse(Fragment::IfBlock , Node ) {
DictParser Dict("If", this);
Dict.unrecognized(
[&](llvm::StringRef) { F.HasUnrecognizedCondition = true; });
Dict.handle("PathMatch", [&](Node ) {
if (auto Values = scalarValues(N))
F.PathMatch = std::move(*Values);
- return !N.failed();
});
-return Dict.parse(N);
+Dict.parse(N);
}
- bool parse(Fragment::CompileFlagsBlock , Node ) {
+ void parse(Fragment::CompileFlagsBlock , Node ) {
DictParser Dict("CompileFlags", this);
Dict.handle("Add", [&](Node ) {
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>> Keys;
+std::vector>> Keys;
std::function 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 Parse) {
+void handle(llvm::StringLiteral Key, std::function Parse) {
for (const auto : 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 ) const {
+void parse(Node ) const {
if (N.getType() != Node::NK_Mapping) {
Outer->error(Description + " should be a dictionary", N);
-return false;
+return;
}
llvm::SmallSet Seen;
+ // We *must* consume all items, even on
