[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