[C++20] Fix crash with invalid concept requirement

[AaronBallman]

We were previously recovering a bit too hard; consumeClose() would skip to a recovery point, then we would call skipToEnd() to skip to another recovery point. Needless to say, the follow-on diagnostics were not great. But also, follow-on diagnostics were crashing due to unexpected null constraint expressions.

Now we only recover once.

Fixes #138820

[llvmbot]

@llvm/pr-subscribers-clang

Author: Aaron Ballman (AaronBallman)

Changes

We were previously recovering a bit too hard; consumeClose() would skip to a recovery point, then we would call skipToEnd() to skip to another recovery point. Needless to say, the follow-on diagnostics were not great. But also, follow-on diagnostics were crashing due to unexpected null constraint expressions.

Now we only recover once.

Fixes #138820

---

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

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+2)
• (modified) clang/lib/Parse/ParseExprCXX.cpp (+3-1)
• (modified) clang/test/SemaCXX/concept-crash-on-diagnostic.cpp (+12)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 55f774f5a672e..350244e3054cf 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -660,6 +660,8 @@ Bug Fixes to C++ Support
 - Fixed a crash when forming an invalid function type in a dependent context. (#GH138657) (#GH115725) (#GH68852)
 - No longer crashes when instantiating invalid variable template specialization
   whose type depends on itself. (#GH51347), (#GH55872)
+- Improved parser recovery of invalid requirement expressions. In turn, this
+  fixes crashes from follow-on processing of the invalid requirement. (#GH138820)
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Parse/ParseExprCXX.cpp b/clang/lib/Parse/ParseExprCXX.cpp
index 32b08a12a3bb6..546c228a30513 100644
--- a/clang/lib/Parse/ParseExprCXX.cpp
+++ b/clang/lib/Parse/ParseExprCXX.cpp
@@ -3706,8 +3706,10 @@ ExprResult Parser::ParseRequiresExpression() {
           SkipUntil(tok::semi, tok::r_brace, SkipUntilFlags::StopBeforeMatch);
           break;
         }
+        // If there's an error consuming the closing bracket, consumeClose()
+        // will handle skipping to the nearest recovery point for us.
         if (ExprBraces.consumeClose())
-          ExprBraces.skipToEnd();
+          break;
 
         concepts::Requirement *Req = nullptr;
         SourceLocation NoexceptLoc;
diff --git a/clang/test/SemaCXX/concept-crash-on-diagnostic.cpp b/clang/test/SemaCXX/concept-crash-on-diagnostic.cpp
index c38f8888075de..1efed72522fef 100644
--- a/clang/test/SemaCXX/concept-crash-on-diagnostic.cpp
+++ b/clang/test/SemaCXX/concept-crash-on-diagnostic.cpp
@@ -48,3 +48,15 @@ concept is_foo_concept = __is_same(foo::bar, T);
 // expected-error@-1 {{'bar' is a private member of 'GH131530::foo'}}
 
 }
+
+namespace GH138820 {
+int a;
+template<typename T>
+concept atomicish = requires() {
+  {    // expected-note {{to match this '{'}}
+    a
+   ... // expected-error {{expected '}'}}
+  };
+};
+atomicish<int> f(); // expected-error {{expected 'auto' or 'decltype(auto)' after concept name}}
+} // namespace GH138820

[zyn0217]

Thanks!

[shafik]

Does this also fix: #138823

[AaronBallman]

Does this also fix: #138823

I don't believe so, at least from my testing on godbolt (which should have my changes, from the git hash they have with --version).