[flang] Improve parser error recovery #173803
Open
+58
−73
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 llvm#173522.
llvmbot
added
flang
Member
|
@llvm/pr-subscribers-flang-parser Author: Peter Klausler (klausler) ChangesA 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 #173522. Full diff: https://github.com/llvm/llvm-project/pull/173803.diff 7 Files Affected:
diff --git a/flang/include/flang/Parser/parse-state.h b/flang/include/flang/Parser/parse-state.h
index 36d70b81b7923..cc787e4105b55 100644
--- a/flang/include/flang/Parser/parse-state.h
+++ b/flang/include/flang/Parser/parse-state.h
@@ -192,19 +192,29 @@ class ParseState {
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:
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index a31eb542a50d0..97edc002fc48f 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -2748,7 +2748,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;
};
diff --git a/flang/lib/Lower/IO.cpp b/flang/lib/Lower/IO.cpp
index cd53dc9482283..c3f9c1a882423 100644
--- a/flang/lib/Lower/IO.cpp
+++ b/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);
}},
diff --git a/flang/lib/Parser/basic-parsers.h b/flang/lib/Parser/basic-parsers.h
index 46d5168c80fe7..c71bf1c3c34ee 100644
--- a/flang/lib/Parser/basic-parsers.h
+++ b/flang/lib/Parser/basic-parsers.h
@@ -391,11 +391,12 @@ template <typename PA, typename PB> class RecoveryParser {
state.set_anyTokenMatched();
}
if (hadDeferredMessages) {
+ CHECK(originallyDeferred);
state.set_anyDeferredMessages();
}
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;
diff --git a/flang/lib/Parser/io-parsers.cpp b/flang/lib/Parser/io-parsers.cpp
index c69ea58738b90..cb3e68a05c94d 100644
--- a/flang/lib/Parser/io-parsers.cpp
+++ b/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{
diff --git a/flang/lib/Parser/unparse.cpp b/flang/lib/Parser/unparse.cpp
index 1fb03c28d10a3..34f79cb15e3b2 100644
--- a/flang/lib/Parser/unparse.cpp
+++ b/flang/lib/Parser/unparse.cpp
@@ -1326,65 +1326,26 @@ class UnparseVisitor {
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(", ");
diff --git a/flang/test/Parser/bug173522.f90 b/flang/test/Parser/bug173522.f90
new file mode 100644
index 0000000000000..bb8321be34f36
--- /dev/null
+++ b/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
|
Member
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Peter Klausler (klausler) ChangesA 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 #173522. Full diff: https://github.com/llvm/llvm-project/pull/173803.diff 7 Files Affected:
diff --git a/flang/include/flang/Parser/parse-state.h b/flang/include/flang/Parser/parse-state.h
index 36d70b81b7923..cc787e4105b55 100644
--- a/flang/include/flang/Parser/parse-state.h
+++ b/flang/include/flang/Parser/parse-state.h
@@ -192,19 +192,29 @@ class ParseState {
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:
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index a31eb542a50d0..97edc002fc48f 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -2748,7 +2748,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;
};
diff --git a/flang/lib/Lower/IO.cpp b/flang/lib/Lower/IO.cpp
index cd53dc9482283..c3f9c1a882423 100644
--- a/flang/lib/Lower/IO.cpp
+++ b/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);
}},
diff --git a/flang/lib/Parser/basic-parsers.h b/flang/lib/Parser/basic-parsers.h
index 46d5168c80fe7..c71bf1c3c34ee 100644
--- a/flang/lib/Parser/basic-parsers.h
+++ b/flang/lib/Parser/basic-parsers.h
@@ -391,11 +391,12 @@ template <typename PA, typename PB> class RecoveryParser {
state.set_anyTokenMatched();
}
if (hadDeferredMessages) {
+ CHECK(originallyDeferred);
state.set_anyDeferredMessages();
}
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;
diff --git a/flang/lib/Parser/io-parsers.cpp b/flang/lib/Parser/io-parsers.cpp
index c69ea58738b90..cb3e68a05c94d 100644
--- a/flang/lib/Parser/io-parsers.cpp
+++ b/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{
diff --git a/flang/lib/Parser/unparse.cpp b/flang/lib/Parser/unparse.cpp
index 1fb03c28d10a3..34f79cb15e3b2 100644
--- a/flang/lib/Parser/unparse.cpp
+++ b/flang/lib/Parser/unparse.cpp
@@ -1326,65 +1326,26 @@ class UnparseVisitor {
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(", ");
diff --git a/flang/test/Parser/bug173522.f90 b/flang/test/Parser/bug173522.f90
new file mode 100644
index 0000000000000..bb8321be34f36
--- /dev/null
+++ b/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
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 #173522.