[flang] Improve parser error recovery (#173803)

A bad I/O control specification ("NML=123" in this case) triggers an
assertion when parsing fails but emits no error message. The message was
lost when combining failed alternative parse states. Make
CombineFailedParses() take more care to not discard messages; then also
strengthen I/O control specification error recovery so that the original
test case doesn't just elicit a confusing complaint about a missing
expected "*" at the site of the "NML" keyword.

Fixes https://github.com/llvm/llvm-project/issues/173522.

flang/include/flang/Parser/parse-state.h

@@ -192,19 +192,29 @@ public:
     return remain;
   }
 
-  void CombineFailedParses(ParseState &&prev) {
-    if (prev.anyTokenMatched_) {
-      if (!anyTokenMatched_ || prev.p_ > p_) {
-        anyTokenMatched_ = true;
-        p_ = prev.p_;
-        messages_ = std::move(prev.messages_);
-      } else if (prev.p_ == p_) {
-        messages_.Merge(std::move(prev.messages_));
-      }
+  void CombineFailedParses(ParseState &&that) {
+    bool haveMessages{anyDeferredMessages_ || !messages_.empty()};
+    bool thatHasMessages{that.anyDeferredMessages_ || !that.messages_.empty()};
+    if (haveMessages && !thatHasMessages) {
+      // never discard messages
+    } else if (!that.anyTokenMatched_ && (haveMessages || !thatHasMessages)) {
+      // token matching is preferred
+    } else if ((!anyTokenMatched_ && that.anyTokenMatched_) ||
+        (!haveMessages && thatHasMessages) ||
+        that.p_ > p_) { // 'that' is better, or no worse and got further
+      anyTokenMatched_ = that.anyTokenMatched_;
+      p_ = that.p_;
+      messages_ = std::move(that.messages_);
+      anyDeferredMessages_ = that.anyDeferredMessages_;
+      anyConformanceViolation_ = that.anyConformanceViolation_;
+      anyErrorRecovery_ = that.anyErrorRecovery_;
+    } else if (that.p_ == p_) { // merge states
+      anyTokenMatched_ |= that.anyTokenMatched_;
+      messages_.Merge(std::move(that.messages_));
+      anyDeferredMessages_ |= that.anyDeferredMessages_;
+      anyConformanceViolation_ |= that.anyConformanceViolation_;
+      anyErrorRecovery_ |= that.anyErrorRecovery_;
     }
-    anyDeferredMessages_ |= prev.anyDeferredMessages_;
-    anyConformanceViolation_ |= prev.anyConformanceViolation_;
-    anyErrorRecovery_ |= prev.anyErrorRecovery_;
   }
 
 private:

flang/include/flang/Parser/parse-tree.h

@@ -2749,7 +2749,8 @@ struct IoControlSpec {
   WRAPPER_CLASS(Rec, ScalarIntExpr);
   WRAPPER_CLASS(Size, ScalarIntVariable);
   std::variant<IoUnit, Format, Name, CharExpr, Asynchronous, EndLabel, EorLabel,
-      ErrLabel, IdVariable, MsgVariable, StatVariable, Pos, Rec, Size>
+      ErrLabel, IdVariable, MsgVariable, StatVariable, Pos, Rec, Size,
+      ErrorRecovery>
       u;
 };
 

flang/lib/Lower/IO.cpp

@@ -1427,6 +1427,9 @@ static void threadSpecs(Fortran::lower::AbstractConverter &converter,
               // already finished.
               return ok;
             },
+            [](const Fortran::parser::ErrorRecovery &) -> mlir::Value {
+              llvm::report_fatal_error("ErrorRecovery in parse tree");
+            },
             [&](const auto &x) {
               return genIOOption(converter, loc, cookie, x);
             }},

flang/lib/Parser/basic-parsers.h

@@ -395,7 +395,7 @@ public:
     }
     if (bx) {
       // Error recovery situations must also produce messages.
-      CHECK(state.anyDeferredMessages() || state.messages().AnyFatalError());
+      CHECK(hadDeferredMessages || state.messages().AnyFatalError());
       state.set_anyErrorRecovery();
     }
     return bx;

flang/lib/Parser/io-parsers.cpp

@@ -231,7 +231,12 @@ TYPE_PARSER(first(construct<IoControlSpec>("UNIT =" >> ioUnit),
         construct<IoControlSpec::CharExpr>(
             pure(IoControlSpec::CharExpr::Kind::Sign), scalarDefaultCharExpr)),
     construct<IoControlSpec>(
-        "SIZE =" >> construct<IoControlSpec::Size>(scalarIntVariable))))
+        "SIZE =" >> construct<IoControlSpec::Size>(scalarIntVariable)),
+    lookAhead(keyword) >>
+        construct<IoControlSpec>(recovery(
+            fail<ErrorRecovery>(
+                "invalid or unknown I/O control specification"_err_en_US),
+            keyword >> "="_tok >> expr >> construct<ErrorRecovery>()))))
 
 // R1211 write-stmt -> WRITE ( io-control-spec-list ) [output-item-list]
 constexpr auto outputItemList{

flang/lib/Parser/unparse.cpp

@@ -1326,65 +1326,26 @@ public:
     Walk(", ", std::get<std::list<OutputItem>>(x.t), ", ");
   }
   bool Pre(const IoControlSpec &x) { // R1213
-    return common::visit(common::visitors{
-                             [&](const IoUnit &) {
-                               Word("UNIT=");
-                               return true;
-                             },
-                             [&](const Format &) {
-                               Word("FMT=");
-                               return true;
-                             },
-                             [&](const Name &) {
-                               Word("NML=");
-                               return true;
-                             },
-                             [&](const IoControlSpec::CharExpr &y) {
-                               Walk(y.t, "=");
-                               return false;
-                             },
-                             [&](const IoControlSpec::Asynchronous &) {
-                               Word("ASYNCHRONOUS=");
-                               return true;
-                             },
-                             [&](const EndLabel &) {
-                               Word("END=");
-                               return true;
-                             },
-                             [&](const EorLabel &) {
-                               Word("EOR=");
-                               return true;
-                             },
-                             [&](const ErrLabel &) {
-                               Word("ERR=");
-                               return true;
-                             },
-                             [&](const IdVariable &) {
-                               Word("ID=");
-                               return true;
-                             },
-                             [&](const MsgVariable &) {
-                               Word("IOMSG=");
-                               return true;
-                             },
-                             [&](const StatVariable &) {
-                               Word("IOSTAT=");
-                               return true;
-                             },
-                             [&](const IoControlSpec::Pos &) {
-                               Word("POS=");
-                               return true;
-                             },
-                             [&](const IoControlSpec::Rec &) {
-                               Word("REC=");
-                               return true;
-                             },
-                             [&](const IoControlSpec::Size &) {
-                               Word("SIZE=");
-                               return true;
-                             },
-                         },
+    common::visit(
+        common::visitors{
+            [&](const IoUnit &) { Word("UNIT="); },
+            [&](const Format &) { Word("FMT="); },
+            [&](const Name &) { Word("NML="); },
+            [&](const IoControlSpec::CharExpr &y) { Walk(y.t, "="); },
+            [&](const IoControlSpec::Asynchronous &) { Word("ASYNCHRONOUS="); },
+            [&](const EndLabel &) { Word("END="); },
+            [&](const EorLabel &) { Word("EOR="); },
+            [&](const ErrLabel &) { Word("ERR="); },
+            [&](const IdVariable &) { Word("ID="); },
+            [&](const MsgVariable &) { Word("IOMSG="); },
+            [&](const StatVariable &) { Word("IOSTAT="); },
+            [&](const IoControlSpec::Pos &) { Word("POS="); },
+            [&](const IoControlSpec::Rec &) { Word("REC="); },
+            [&](const IoControlSpec::Size &) { Word("SIZE="); },
+            [&](const ErrorRecovery &) {},
+        },
         x.u);
+    return true;
   }
   void Unparse(const InputImpliedDo &x) { // R1218
     Put('('), Walk(std::get<std::list<InputItem>>(x.t), ", "), Put(", ");

flang/test/Parser/bug173522.f90

@@ -0,0 +1,4 @@
+!RUN: not %flang_fc1 -fsyntax-only %s 2>&1 | FileCheck %s
+!CHECK: 3:10: error: invalid or unknown I/O control specification
+write (*,nml=123)
+end