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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
