[clang] Fix eager skipping on new with unknown type and no new-initializer

[alejandro-alvarez-sonarsource]

i.e., in function(new Unknown); the parser should skip only until the semicolon.

Before this change, everything was skipped until a balanced closing parenthesis or brace was found. This strategy can cause completely bogus ASTs. For instance, in the case of the test new-unknown-type.cpp, struct Bar would end nested under the namespace a::b.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Alejandro Álvarez Ayllón (alejandro-alvarez-sonarsource)

Changes

i.e., in function(new Unknown); the parser should skip only until the semicolon.

Before this change, everything was skipped until a balanced closing parenthesis or brace was found. This strategy can cause completely bogus ASTs. For instance, in the case of the test new-unknown-type.cpp, struct Bar would end nested under the namespace a::b.

---

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

4 Files Affected:

• (modified) clang/lib/Parse/ParseExpr.cpp (+1-1)
• (added) clang/test/AST/new-unknown-type.cpp (+25)
• (modified) clang/test/Parser/colon-colon-parentheses.cpp (+1-1)
• (modified) clang/test/Parser/cxx-ambig-paren-expr-asan.cpp (-1)

diff --git a/clang/lib/Parse/ParseExpr.cpp b/clang/lib/Parse/ParseExpr.cpp
index e7514500dc53a4..2fb4be0035b667 100644
--- a/clang/lib/Parse/ParseExpr.cpp
+++ b/clang/lib/Parse/ParseExpr.cpp
@@ -3694,7 +3694,7 @@ bool Parser::ParseExpressionList(SmallVectorImpl<Expr *> &Exprs,
       SawError = true;
       if (FailImmediatelyOnInvalidExpr)
         break;
-      SkipUntil(tok::comma, tok::r_paren, StopBeforeMatch);
+      SkipUntil(tok::comma, tok::r_paren, StopAtSemi | StopBeforeMatch);
     } else {
       Exprs.push_back(Expr.get());
     }
diff --git a/clang/test/AST/new-unknown-type.cpp b/clang/test/AST/new-unknown-type.cpp
new file mode 100644
index 00000000000000..e7dd3c90145d25
--- /dev/null
+++ b/clang/test/AST/new-unknown-type.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -verify -ast-dump %s | FileCheck %s
+
+extern void foo(Unknown*); // expected-error {{unknown type name 'Unknown'}}
+
+namespace a {
+  void computeSomething() {
+    foo(new Unknown()); // expected-error {{unknown type name 'Unknown'}}
+    foo(new Unknown{}); // expected-error {{unknown type name 'Unknown'}}
+    foo(new Unknown);   // expected-error {{unknown type name 'Unknown'}}
+  }
+} // namespace a
+
+namespace b {
+  struct Bar{};
+} // namespace b
+
+// CHECK:      |-NamespaceDecl 0x{{[^ ]*}} <line:5:1, line:11:1> line:5:11 a
+// CHECK-NEXT: | `-FunctionDecl 0x{{[^ ]*}} <line:6:3, line:10:3> line:6:8 computeSomething 'void ()'
+// CHECK-NEXT: |   `-CompoundStmt 0x{{[^ ]*}} <col:27, line:10:3>
+// CHECK-NEXT: |-NamespaceDecl 0x{{[^ ]*}} <line:13:1, line:15:1> line:13:11 b
+// CHECK-NEXT: | `-CXXRecordDecl 0x{{[^ ]*}} <line:14:3, col:14> col:10 referenced struct Bar definition
+
+static b::Bar bar;
+// CHECK:      `-VarDecl 0x{{[^ ]*}} <line:23:1, col:15> col:15 bar 'b::Bar' static callinit
+// CHECK-NEXT:   `-CXXConstructExpr 0x{{[^ ]*}} <col:15> 'b::Bar' 'void () noexcept'
diff --git a/clang/test/Parser/colon-colon-parentheses.cpp b/clang/test/Parser/colon-colon-parentheses.cpp
index b8e203c935990a..018fa7f20e5210 100644
--- a/clang/test/Parser/colon-colon-parentheses.cpp
+++ b/clang/test/Parser/colon-colon-parentheses.cpp
@@ -11,7 +11,7 @@ int S::(*e;  // expected-error{{expected unqualified-id}}
 int S::*f;
 int g = S::(a);  // expected-error {{expected unqualified-id}} expected-error {{use of undeclared identifier 'a'}}
 int h = S::(b;  // expected-error {{expected unqualified-id}} expected-error {{use of undeclared identifier 'b'}}
-            );
+            );  // expected-error {{expected unqualified-id}}
 int i = S::c;
 
 void foo() {
diff --git a/clang/test/Parser/cxx-ambig-paren-expr-asan.cpp b/clang/test/Parser/cxx-ambig-paren-expr-asan.cpp
index ec9d6b9da39d09..7b9e51f12c95e5 100644
--- a/clang/test/Parser/cxx-ambig-paren-expr-asan.cpp
+++ b/clang/test/Parser/cxx-ambig-paren-expr-asan.cpp
@@ -6,4 +6,3 @@ int H((int()[)]);
 // expected-error@-1 {{expected expression}}
 // expected-error@-2 {{expected ']'}}
 // expected-note@-3 {{to match this '['}}
-// expected-error@-4 {{expected ';' after top level declarator}}

[Sirraide]

LGTM.

ParseExpressionList() is used in a lot of places, but given that almost all the other tests seem to have no problem with this change (which honestly surprises me a bit), I feel like this is reasonable (in the tests that are affected, we gain a bogus diagnostic in one and lose one in another, so imo that evens out ;Þ), and a more accurate AST in is always nice.