[flang] Improve syntax error messages by fixing withMessage() parser combinator

[klausler]

Summary

The parser combinator withMessage("error message"_err_en_US, PARSER) is meant
to run the parser PARSER and, if it fails, override its error messages if
it failed silently or it was unable to recognize any tokens at all. This
gives the parser a way to avoid emitting some confusing or missing error
messages. Unfortunately, the implementation could sometimes lose track of
whether any tokens had been recognized, leading to problems with outer usage
of withMessage() and also -- more seriously -- with ParseState::CombineFailedParses().
That's a utility that determines which error messages to retain when two
or more parsers have been attempted at the same starting point and none
of them succceed. Its policy is to retain the state from the parser that
consumed the most input text before failing, so long as it had recognized at
least one token.

So anyway, fix up withMessage(), adjust the tests, and add a test of the
original motivating confusing error situation, in which a syntax error in
a COMMON statement was being diagnosed as a problem with a statement function
definition because withMessage() had lost the fact that the parse of the
COMMON statement had recognized some tokens, and the last attempted parse
later was a failed attempt to parse a statement function.

[clementval]

LGTM

flang/lib/Parser/basic-parsers.h

 // has matched no tokens.
 template <typename PA> class WithMessageParser {
 public:
   using resultType = typename PA::resultType;
   constexpr WithMessageParser(const WithMessageParser &) = default;
   constexpr WithMessageParser(MessageFixedText t, PA p)
       : text_{t}, parser_{p} {}
   std::optional<resultType> Parse(ParseState &state) const {
+    if (state.deferMessages()) { // fast path
+      std::optional<resultType> result{parser_.Parse(state)};
+      if (!result) {
+        state.set_anyDeferredMessages();
+      }
+      return result;
+    }
     Messages messages{std::move(state.messages())};
-    ParseState backtrack{state};
+    bool hadAnyTokenMatched{state.anyTokenMatched()};
     state.set_anyTokenMatched(false);
     std::optional<resultType> result{parser_.Parse(state)};
     bool emitMessage{false};
     if (result) {
       messages.Annex(std::move(state.messages()));
-      if (backtrack.anyTokenMatched()) {
+      if (hadAnyTokenMatched) {
         state.set_anyTokenMatched();
       }
     } else if (state.anyTokenMatched()) {
       emitMessage = state.messages().empty();
       messages.Annex(std::move(state.messages()));
-      backtrack.set_anyTokenMatched();
-      if (state.anyDeferredMessages()) {
-        backtrack.set_anyDeferredMessages(true);
-      }
-      state = std::move(backtrack);
     } else {
       emitMessage = true;
+      if (hadAnyTokenMatched) {
+        state.set_anyTokenMatched();
+      }
     }
     state.messages() = std::move(messages);
     if (emitMessage) {
       state.Say(text_);
     }
     return result;
   }
 

 // a does so, or if a fails and b succeeds.  If a succeeds, b is not attempted.
 // All messages from the first parse are retained.
 // The two parsers must return values of the same type.
 template <typename PA, typename PB> class RecoveryParser {
 public:
   using resultType = typename PA::resultType;
   static_assert(std::is_same_v<resultType, typename PB::resultType>);
   constexpr RecoveryParser(const RecoveryParser &) = default;
-  constexpr RecoveryParser(PA pa, PB pb) : pa_{pa}, pb3_{pb} {}
+  constexpr RecoveryParser(PA pa, PB pb) : pa_{pa}, pb_{pb} {}
   std::optional<resultType> Parse(ParseState &state) const {
     bool originallyDeferred{state.deferMessages()};
     ParseState backtrack{state};
     if (!originallyDeferred && state.messages().empty() &&
         !state.anyErrorRecovery()) {
       // Fast path.  There are no messages or recovered errors in the incoming
       // state.  Attempt to parse with messages deferred, expecting that the
       // parse will succeed silently.
if (std::optional<resultType> ax{pa_.Parse(state)}) {
       state.messages().Annex(std::move(messages));
       return ax;
     }
     messages.Annex(std::move(state.messages()));
     bool hadDeferredMessages{state.anyDeferredMessages()};
     bool anyTokenMatched{state.anyTokenMatched()};
     state = std::move(backtrack);
     state.set_deferMessages(true);
-    std::optional<resultType> bx{pb3_.Parse(state)};
+    std::optional<resultType> bx{pb_.Parse(state)};
     state.messages() = std::move(messages);
     state.set_deferMessages(originallyDeferred);
     if (anyTokenMatched) {
       state.set_anyTokenMatched();
     }
     if (hadDeferredMessages) {
       state.set_anyDeferredMessages();
     }
     if (bx) {
       // Error recovery situations must also produce messages.
       CHECK(state.anyDeferredMessages() || state.messages().AnyFatalError());
       state.set_anyErrorRecovery();
     }
     return bx;
   }
 
 private:
   const PA pa_;
-  const PB pb3_;
+  const PB pb_;
 };
 
 template <typename PA, typename PB>
 inline constexpr auto recovery(PA pa, PB pb) {
   return RecoveryParser<PA, PB>{pa, pb};
 }
 
 // If x is a parser, then many(x) returns a parser that always succeeds

flang/test/Driver/color-diagnostics-parse.f90

 ! Test the behaviors of -f{no-}color-diagnostics when emitting parsing
 ! diagnostics.
 ! Windows command prompt doesn't support ANSI escape sequences.
 ! REQUIRES: shell
 
 ! RUN: not %flang %s -fcolor-diagnostics 2>&1 \
 ! RUN:     | FileCheck %s --check-prefix=CHECK_CD
 ! RUN: not %flang %s -fno-color-diagnostics 2>&1 \
 ! RUN:     | FileCheck %s --check-prefix=CHECK_NCD
 ! RUN: not %flang_fc1 %s -fcolor-diagnostics 2>&1 \
 ! RUN:     | FileCheck %s --check-prefix=CHECK_CD
 ! RUN: not %flang_fc1 %s 2>&1 | FileCheck %s --check-prefix=CHECK_NCD
 
-! CHECK_CD: {{.*}}[0;1;31merror: {{.*}}[0mexpected '('
+! CHECK_CD: {{.*}}[0;1;31merror: {{.*}}[0mexpected end of statement
 
-! CHECK_NCD: error: expected '('
+! CHECK_NCD: error: expected end of statement
 
 program m
   integer :: i =
 end

flang/test/Parser/doubled-comma.f90

+! RUN: not %flang_fc1 -fsyntax-only %s 2>&1 | FileCheck %s
+! CHECK: 3:13: error: expected end of statement
+common/blk/a,,b
+end

flang/test/Semantics/error_stop1a.f90

program test_error_stop
   error stop int_code, quiet=logical_coarray
   error stop int_code, quiet=logical_coarray[1]
   error stop int_code, quiet=.true.
   error stop (int_code), quiet=.false.
 
   !___ non-standard-conforming statements _________________________
 
   ! unknown stop-code
-  !ERROR: expected execution part construct
+  !ERROR: expected end of statement
   error stop code=int_code
 
   ! missing 'quiet='
-  !ERROR: expected execution part construct
+  !ERROR: expected end of statement
   error stop int_code, bool
 
   ! incorrect spelling for 'quiet='
-  !ERROR: expected execution part construct
+  !ERROR: expected end of statement
   error stop int_code, quiets=bool
 
   ! missing scalar-logical-expr for quiet=
-  !ERROR: expected execution part construct
+  !ERROR: expected end of statement
   error stop int_code, quiet
 
   ! superfluous stop-code
-  !ERROR: expected execution part construct
+  !ERROR: expected end of statement
   error stop int_code, char_code
 
   ! repeated quiet=
-  !ERROR: expected execution part construct
+  !ERROR: expected end of statement
   error stop int_code, quiet=bool, quiet=.true.
 
   ! superfluous stop-code
-  !ERROR: expected execution part construct
+  !ERROR: expected end of statement
   error stop int_code, char_code, quiet=bool
 
   ! superfluous integer
-  !ERROR: expected execution part construct
+  !ERROR: expected end of statement
   error stop int_code, quiet=bool, 5
 
   ! quiet= appears without stop-code
-  !ERROR: expected execution part construct
+  !ERROR: expected end of statement
   error stop quiet=bool
 
   ! incorrect syntax
-  !ERROR: expected execution part construct
+  !ERROR: expected end of statement
   error stop ()
 
   ! incorrect syntax
-  !ERROR: expected execution part construct
+  !ERROR: expected end of statement
   error stop (2, quiet=.true.)
 
 end program test_error_stop

flang/test/Semantics/synchronization01a.f90

program test_sync_all
   sync all(stat=sync_status)
   sync all(                  errmsg=error_message)
   sync all(stat=sync_status, errmsg=error_message)
 
   !___ non-standard-conforming statement ___
 
   !______ invalid sync-stat-lists: invalid stat= ____________
 
-  !ERROR: expected execution part construct
+  !ERROR: expected end of statement
   sync all(status=sync_status)
 
   ! Invalid sync-stat-list: missing stat-variable
-  !ERROR: expected execution part construct
+  !ERROR: expected end of statement
   sync all(stat)
 
   ! Invalid sync-stat-list: missing 'stat='
-  !ERROR: expected execution part construct
+  !ERROR: expected end of statement
   sync all(sync_status)
 
   !______ invalid sync-stat-lists: invalid errmsg= ____________
 
   ! Invalid errmsg-variable keyword
-  !ERROR: expected execution part construct
+  !ERROR: expected end of statement
   sync all(errormsg=error_message)
 
   ! Invalid sync-stat-list: missing 'errmsg='
-  !ERROR: expected execution part construct
+  !ERROR: expected end of statement
   sync all(error_message)
 
   ! Invalid sync-stat-list: missing errmsg-variable
-  !ERROR: expected execution part construct
+  !ERROR: expected end of statement
   sync all(errmsg)
 
 end program test_sync_all

flang/test/Semantics/synchronization03a.f90

program test_sync_memory
   sync memory(stat=sync_status)
   sync memory(                  errmsg=error_message)
   sync memory(stat=sync_status, errmsg=error_message)
 
   !___ non-standard-conforming statements ___
 
   !______ invalid sync-stat-lists: invalid stat= ____________
 
-  !ERROR: expected execution part construct
+  !ERROR: expected end of statement
   sync memory(status=sync_status)
 
   ! Invalid sync-stat-list: missing stat-variable
-  !ERROR: expected execution part construct
+  !ERROR: expected end of statement
   sync memory(stat)
 
   ! Invalid sync-stat-list: missing 'stat='
-  !ERROR: expected execution part construct
+  !ERROR: expected end of statement
   sync memory(sync_status)
 
   !______ invalid sync-stat-lists: invalid errmsg= ____________
 
   ! Invalid errmsg-variable keyword
-  !ERROR: expected execution part construct
+  !ERROR: expected end of statement
   sync memory(errormsg=error_message)
 
   ! Invalid sync-stat-list: missing 'errmsg='
-  !ERROR: expected execution part construct
+  !ERROR: expected end of statement
   sync memory(error_message)
 
   ! Invalid sync-stat-list: missing errmsg-variable
-  !ERROR: expected execution part construct
+  !ERROR: expected end of statement
   sync memory(errmsg)
 
 end program test_sync_memory