[clangd] Fix error handling in config.yaml parsing.

[sammccall]

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

[hokein]

thanks.

clang-tools-extra/clangd/ConfigYAML.cpp

36

nit: the comment is stale.

clang-tools-extra/clangd/ConfigYAML.cpp

 using llvm::yaml::BlockScalarNode;
 using llvm::yaml::MappingNode;
 using llvm::yaml::Node;
 using llvm::yaml::ScalarNode;
 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.
   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;
 
   public:
     DictParser(llvm::StringRef Description, Parser *Outer)
         : Description(Description), Outer(Outer) {}
 
     // 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");
       }
       Keys.emplace_back(Key, std::move(Parse));
     }
 
     // Fallback is called when a Key is not matched by any handle().
     // A warning is also automatically emitted.
     void unrecognized(std::function<void(llvm::StringRef)> Fallback) {
       Unknown = std::move(Fallback);
     }
 
     // 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;
         if (!Seen.insert(**Key).second) {
           Outer->warning("Duplicate key " + **Key + " is ignored", *K);
           continue;
         }
         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;
           }
         }
         if (!Matched) {
           Outer->warning("Unknown " + Description + " key " + **Key, *K);
           if (Unknown)
             Unknown(**Key);
         }
       }
-      return true;
     }
   };
 
   // Try to parse a single scalar value from the node, warn on failure.
   llvm::Optional<Located<std::string>> scalarValue(Node &N,
                                                    llvm::StringRef Desc) {
     llvm::SmallString<256> Buf;
     if (auto *S = llvm::dyn_cast<ScalarNode>(&N))
       return Located<std::string>(S->getValue(Buf).str(), N.getSourceRange());
     if (auto *BS = llvm::dyn_cast<BlockScalarNode>(&N))
       return Located<std::string>(BS->getValue().str(), N.getSourceRange());
     warning(Desc + " should be scalar", N);
     return llvm::None;
   }
 
   // Try to parse a list of single scalar values, or just a single value.
   llvm::Optional<std::vector<Located<std::string>>> scalarValues(Node &N) {
     std::vector<Located<std::string>> Result;
     if (auto *S = llvm::dyn_cast<ScalarNode>(&N)) {
       llvm::SmallString<256> Buf;
       Result.emplace_back(S->getValue(Buf).str(), N.getSourceRange());
     } 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));
       }
     } else {
       warning("Expected scalar or list of scalars", N);
       return llvm::None;
     }
     return Result;
   }
 
   // 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());
   }
 
   // Report a "soft" error that could be caused by e.g. version skew.
   void warning(const llvm::Twine &Msg, const Node &N) {
     SM.PrintMessage(N.getSourceRange().Start, llvm::SourceMgr::DK_Warning, Msg,
                     N.getSourceRange());
std::vector<Fragment> Fragment::parseYAML(llvm::StringRef YAML,
   // Callback receives both errors we emit and those from the YAML parser.
   SM->setDiagHandler(
       [](const llvm::SMDiagnostic &Diag, void *Ctx) {
         (*reinterpret_cast<DiagnosticCallback *>(Ctx))(Diag);
       },
       &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;
       if (Parser(*SM).parse(Fragment, *N))
         Result.push_back(std::move(Fragment));
     }
   }
   // Hack: stash the buffer in the SourceMgr to keep it alive.
   // SM has two entries: "main" non-owning buffer, and ignored owning buffer.
   SM->AddNewSourceBuffer(std::move(Buf), llvm::SMLoc());
   return Result;
 }
 
 } // namespace config
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp

ASSERT_THAT(
       ElementsAre(AllOf(DiagMessage("Unknown If key UnknownCondition"),
                         DiagKind(llvm::SourceMgr::DK_Warning),
                         DiagPos(YAML.range().start), DiagRange(YAML.range())),
                   AllOf(DiagMessage("Unexpected token. Expected Key, Flow "
                                     "Entry, or Flow Mapping End."),
                         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
 } // namespace config
 } // namespace clangd
 } // namespace clang