[flang] Better recovery from errors in a loop control

[klausler]

When there's an error in a DO statement loop control, error recovery isn't great. A bare "DO" is a valid statement, so a failure to parse its loop control doesn't fail on the whole statement. Its partial parse ends after the keyword, and as some other statement parsers can get further into the input before failing, errors in the loop control can lead to confusing error messages about bad pointer assignment statements and others. So just check that a bare "DO" is followed by the end of the statement.

[llvmbot]

@llvm/pr-subscribers-flang-parser

Author: Peter Klausler (klausler)

Changes

When there's an error in a DO statement loop control, error recovery isn't great. A bare "DO" is a value statement, so a failure to parse its loop control doesn't fail on the whole statement. Its partial parse ends after the keyword, and as some other statement parsers can get further into the input before failing, errors in the loop control can lead to confusing error messages about bad pointer assignment statements and others. So just check that a bare "DO" is followed by the end of the statement.

---

Full diff: https://github.com/llvm/llvm-project/pull/117025.diff

3 Files Affected:

• (modified) flang/lib/Parser/executable-parsers.cpp (+11-4)
• (modified) flang/lib/Parser/type-parsers.h (-1)
• (added) flang/test/Parser/recovery07.f90 (+6)

diff --git a/flang/lib/Parser/executable-parsers.cpp b/flang/lib/Parser/executable-parsers.cpp
index 730165613d91db..00ab03d6f86b55 100644
--- a/flang/lib/Parser/executable-parsers.cpp
+++ b/flang/lib/Parser/executable-parsers.cpp
@@ -9,7 +9,6 @@
 // Per-type parsers for executable statements
 
 #include "basic-parsers.h"
-#include "debug-parser.h"
 #include "expr-parsers.h"
 #include "misc-parsers.h"
 #include "stmt-parser.h"
@@ -282,18 +281,26 @@ TYPE_CONTEXT_PARSER("loop control"_en_US,
                 "CONCURRENT" >> concurrentHeader,
                 many(Parser<LocalitySpec>{})))))
 
+// "DO" is a valid statement, so the loop control is optional; but for
+// better recovery from errors in the loop control, don't parse a
+// DO statement with a bad loop control as a DO statement that has
+// no loop control and is followed by garbage.
+static constexpr auto loopControlOrEndOfStmt{
+    construct<std::optional<LoopControl>>(Parser<LoopControl>{}) ||
+    lookAhead(":\n"_ch) >> construct<std::optional<LoopControl>>()};
+
 // R1121 label-do-stmt -> [do-construct-name :] DO label [loop-control]
 // A label-do-stmt with a do-construct-name is parsed as a nonlabel-do-stmt
 // with an optional label.
 TYPE_CONTEXT_PARSER("label DO statement"_en_US,
-    construct<LabelDoStmt>("DO" >> label, maybe(loopControl)))
+    construct<LabelDoStmt>("DO" >> label, loopControlOrEndOfStmt))
 
 // R1122 nonlabel-do-stmt -> [do-construct-name :] DO [loop-control]
 TYPE_CONTEXT_PARSER("nonlabel DO statement"_en_US,
     construct<NonLabelDoStmt>(
-        name / ":", "DO" >> maybe(label), maybe(loopControl)) ||
+        name / ":", "DO" >> maybe(label), loopControlOrEndOfStmt) ||
         construct<NonLabelDoStmt>(construct<std::optional<Name>>(),
-            construct<std::optional<Label>>(), "DO" >> maybe(loopControl)))
+            construct<std::optional<Label>>(), "DO" >> loopControlOrEndOfStmt))
 
 // R1132 end-do-stmt -> END DO [do-construct-name]
 TYPE_CONTEXT_PARSER("END DO statement"_en_US,
diff --git a/flang/lib/Parser/type-parsers.h b/flang/lib/Parser/type-parsers.h
index adbf6d23cbd99a..f800398a22de9a 100644
--- a/flang/lib/Parser/type-parsers.h
+++ b/flang/lib/Parser/type-parsers.h
@@ -102,7 +102,6 @@ constexpr Parser<ForallAssignmentStmt> forallAssignmentStmt; // R1053
 constexpr Parser<ForallStmt> forallStmt; // R1055
 constexpr Parser<Selector> selector; // R1105
 constexpr Parser<EndSelectStmt> endSelectStmt; // R1143 & R1151 & R1155
-constexpr Parser<LoopControl> loopControl; // R1123
 constexpr Parser<ConcurrentHeader> concurrentHeader; // R1125
 constexpr Parser<IoUnit> ioUnit; // R1201, R1203
 constexpr Parser<FileUnitNumber> fileUnitNumber; // R1202
diff --git a/flang/test/Parser/recovery07.f90 b/flang/test/Parser/recovery07.f90
new file mode 100644
index 00000000000000..3a66779b595819
--- /dev/null
+++ b/flang/test/Parser/recovery07.f90
@@ -0,0 +1,6 @@
+! RUN: not %flang_fc1 -fsyntax-only %s 2>&1 | FileCheck %s
+! CHECK: error: expected ':'
+! CHECK: in the context: loop control
+do concurrent(I = 1, N)
+end do
+end

[psteinfeld]

All builds and tests correctly and looks good.

[eugeneepshteyn]

So this is trying to construct parsed loop control, and failing that it'll construct an empty loop control.
Could you please help me understand lookAhead(":\n"_ch)? Why is there : and what's the significance of user defined literal _ch?

[eugeneepshteyn]

Ok, I figured out _ch part, but still have a question about why : is used in lookAhead()

[klausler]

The ':' (colon) is a typo, and should be a ';' semicolon; will fix. It's checking for the end of the statement.