Bug 30934 - YAML error not reported by ``yaml::Input::error()``

[serge-sans-paille]

@mehdi_amini up :-)

[mehdi_amini]

lib/Support/YAMLTraits.cpp

115

So it seems that any error checking on Input should also check on the Stream itself. It may be less error prone to refactor this check into a single method and turn all the tests into if (hasError()) return;

[serge-sans-paille]

Refactoring done!

[serge-sans-paille]

lib/Support/YAMLTraits.cpp

115

That's my feeling too. I did the proposed refactoring, putting the method as private to prevent semantic conflict with the public error() method.

[mehdi_amini]

Thanks, LGTM.

[mehdi_amini]

Serge: the unittest is not passing, can you have a look?

[serge-sans-paille]

This patch seemsmore robust: when a subcomponent reports an error, it may set the parent error_code.

[serge-sans-paille]

Different system, different header deps; I made this one explicit.

include/llvm/Support/YAMLTraits.h

private:
   bool matchEnumFallback() override;
   void endEnumScalar() override;
   bool beginBitSetScalar(bool &) override;
   bool bitSetMatch(const char *, bool ) override;
   void endBitSetScalar() override;
   void scalarString(StringRef &, bool) override;
   void blockScalarString(StringRef &) override;
   void setError(const Twine &message) override;
+  bool hasError() const;
   bool canElideEmptySequence() override;
 
   class HNode {
     virtual void anchor();
   public:
     HNode(Node *n) : _node(n) { }
     virtual ~HNode() { }
     static inline bool classof(const HNode *) { return true; }

lib/Support/YAMLTraits.cpp

if (foundTag.empty()) {
     // If no tag found and 'Tag' is the default, say it was found.
     return Default;
   }
   // Return true iff found tag matches supplied tag.
   return Tag.equals(foundTag);
 }
 
 void Input::beginMapping() {
-  if (EC)
+  if (hasError())
     return;
   // CurrentNode can be null if the document is empty.
   MapHNode *MN = dyn_cast_or_null<MapHNode>(CurrentNode);
   if (MN) {
     MN->ValidKeys.clear();
   }
 }
 
 bool Input::preflightKey(const char *Key, bool Required, bool, bool &UseDefault,
                          void *&SaveInfo) {
   UseDefault = false;
-  if (EC)
+  if (hasError())
     return false;
 
   // CurrentNode is null for empty documents, which is an error in case required
   // nodes are present.
   if (!CurrentNode) {
     if (Required)
       EC = make_error_code(errc::invalid_argument);
     return false;
bool Input::preflightKey(const char *Key, bool Required, bool, bool &UseDefault,
   return true;
 }
 
 void Input::postflightKey(void *saveInfo) {
   CurrentNode = reinterpret_cast<HNode *>(saveInfo);
 }
 
 void Input::endMapping() {
-  if (EC)
+  if (hasError())
     return;
   // CurrentNode can be null if the document is empty.
   MapHNode *MN = dyn_cast_or_null<MapHNode>(CurrentNode);
   if (!MN)
     return;
   for (const auto &NN : MN->Mapping) {
     if (!MN->isValidKey(NN.first())) {
       setError(NN.second.get(), Twine("unknown key '") + NN.first() + "'");
unsigned Input::beginSequence() {
   setError(CurrentNode, "not a sequence");
   return 0;
 }
 
 void Input::endSequence() {
 }
 
 bool Input::preflightElement(unsigned Index, void *&SaveInfo) {
-  if (EC)
+  if (hasError())
     return false;
   if (SequenceHNode *SQ = dyn_cast<SequenceHNode>(CurrentNode)) {
     SaveInfo = CurrentNode;
     CurrentNode = SQ->Entries[Index].get();
     return true;
   }
   return false;
 }
 
 void Input::postflightElement(void *SaveInfo) {
   CurrentNode = reinterpret_cast<HNode *>(SaveInfo);
 }
 
 unsigned Input::beginFlowSequence() { return beginSequence(); }
 
 bool Input::preflightFlowElement(unsigned index, void *&SaveInfo) {
-  if (EC)
+  if (hasError())
     return false;
   if (SequenceHNode *SQ = dyn_cast<SequenceHNode>(CurrentNode)) {
     SaveInfo = CurrentNode;
     CurrentNode = SQ->Entries[index].get();
     return true;
   }
   return false;
 }
bool Input::beginBitSetScalar(bool &DoClear) {
   } else {
     setError(CurrentNode, "expected sequence of bit values");
   }
   DoClear = true;
   return true;
 }
 
 bool Input::bitSetMatch(const char *Str, bool) {
-  if (EC)
+  if (hasError())
     return false;
   if (SequenceHNode *SQ = dyn_cast<SequenceHNode>(CurrentNode)) {
     unsigned Index = 0;
     for (auto &N : SQ->Entries) {
       if (ScalarHNode *SN = dyn_cast<ScalarHNode>(N.get())) {
         if (SN->value().equals(Str)) {
           BitValuesUsed[Index] = true;
           return true;
         }
       } else {
         setError(CurrentNode, "unexpected scalar in sequence of bit values");
       }
       ++Index;
     }
   } else {
     setError(CurrentNode, "expected sequence of bit values");
   }
   return false;
 }
 
 void Input::endBitSetScalar() {
-  if (EC)
+  if (hasError())
     return;
   if (SequenceHNode *SQ = dyn_cast<SequenceHNode>(CurrentNode)) {
     assert(BitValuesUsed.size() == SQ->Entries.size());
     for (unsigned i = 0; i < SQ->Entries.size(); ++i) {
       if (!BitValuesUsed[i]) {
         setError(SQ->Entries[i].get(), "unknown bit value");
         return;
       }

 
 void Input::blockScalarString(StringRef &S) { scalarString(S, false); }
 
 void Input::setError(HNode *hnode, const Twine &message) {
   assert(hnode && "HNode must not be NULL");
   this->setError(hnode->_node, message);
 }
 
+bool Input::hasError() const {
+  return EC || Strm->failed();
+}
+
 void Input::setError(Node *node, const Twine &message) {
   Strm->printError(node, message);
   EC = make_error_code(errc::invalid_argument);
 }
 
 std::unique_ptr<Input::HNode> Input::createHNodes(Node *N) {
   SmallString<128> StringStorage;
   if (ScalarNode *SN = dyn_cast<ScalarNode>(N)) {
     StringRef KeyStr = SN->getValue(StringStorage);
     if (!StringStorage.empty()) {
       // Copy string to permanent storage
       KeyStr = StringStorage.str().copy(StringAllocator);
     }
     return llvm::make_unique<ScalarHNode>(N, KeyStr);
   } else if (BlockScalarNode *BSN = dyn_cast<BlockScalarNode>(N)) {
     StringRef ValueCopy = BSN->getValue().copy(StringAllocator);
     return llvm::make_unique<ScalarHNode>(N, ValueCopy);
   } else if (SequenceNode *SQ = dyn_cast<SequenceNode>(N)) {
     auto SQHNode = llvm::make_unique<SequenceHNode>(N);
     for (Node &SN : *SQ) {
       auto Entry = this->createHNodes(&SN);
-      if (EC)
+      if (hasError())
         break;
       SQHNode->Entries.push_back(std::move(Entry));
     }
     return std::move(SQHNode);
   } else if (MappingNode *Map = dyn_cast<MappingNode>(N)) {
     auto mapHNode = llvm::make_unique<MapHNode>(N);
     for (KeyValueNode &KVN : *Map) {
       Node *KeyNode = KVN.getKey();
       ScalarNode *KeyScalar = dyn_cast<ScalarNode>(KeyNode);
       if (!KeyScalar) {
         setError(KeyNode, "Map key must be a scalar");
         break;
       }
       StringStorage.clear();
       StringRef KeyStr = KeyScalar->getValue(StringStorage);
       if (!StringStorage.empty()) {
         // Copy string to permanent storage
         KeyStr = StringStorage.str().copy(StringAllocator);
       }
       auto ValueHNode = this->createHNodes(KVN.getValue());
-      if (EC)
+      if (hasError())
         break;
       mapHNode->Mapping[KeyStr] = std::move(ValueHNode);
     }
     return std::move(mapHNode);
   } else if (isa<NullNode>(N)) {
     return llvm::make_unique<EmptyHNode>(N);
   } else {
     setError(N, "unknown node kind");

unittests/Support/YAMLIOTest.cpp

EXPECT_EQ("---\n"
             "Simple:          \n"
             "  B:               2\n"
             "  C:               3\n"
             "  Context:         2\n"
             "...\n",
             out);
   out.clear();
 }
+
+TEST(YAMLIO, InvalidInput) {
+  // polluting 1 value in the sequence
+  Input yin("---\n- foo:  3\n  bar:  5\n1\n- foo:  3\n  bar:  5\n...\n");
+  std::vector<FooBar> Data;
+  yin >> Data;
+  EXPECT_TRUE((bool)yin.error());
+}