From 60d373810230b1cc6246122e1c63ab0eccb8bbf5 Mon Sep 17 00:00:00 2001
From: Ben Clayton <bclayton@google.com>
Date: Tue, 21 Feb 2023 22:01:48 +0000
Subject: [PATCH] tint: Fix ICE in ast::TemplatedIdentifier ctor

Invalid programs could attempt to construct an ast::TemplatedIdentifier with no template arguments.
In this situation an ast::Identifier should be constructed instead.

Bug: chromium:1417465
Change-Id: Id1516cd83679947b5346c69ce5453d72f4f93b49
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/120841
Reviewed-by: Dan Sinclair <dsinclair@chromium.org>
Commit-Queue: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
---
 src/tint/ast/templated_identifier_test.cc     | 24 +++++++++++--------
 src/tint/program_builder.h                    | 17 ++++++-------
 .../reader/wgsl/parser_impl_error_msg_test.cc |  8 +++++++
 3 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/src/tint/ast/templated_identifier_test.cc b/src/tint/ast/templated_identifier_test.cc
index 648c3349ad..ce62c7983d 100644
--- a/src/tint/ast/templated_identifier_test.cc
+++ b/src/tint/ast/templated_identifier_test.cc
@@ -26,21 +26,25 @@ using TemplatedIdentifierTest = TestHelper;
 TEST_F(TemplatedIdentifierTest, Creation) {
     auto* i = Ident("ident", 1_a, Add("x", "y"), false, "x");
     EXPECT_EQ(i->symbol, Symbols().Get("ident"));
-    ASSERT_EQ(i->arguments.Length(), 4u);
-    EXPECT_TRUE(i->arguments[0]->Is<ast::IntLiteralExpression>());
-    EXPECT_TRUE(i->arguments[1]->Is<ast::BinaryExpression>());
-    EXPECT_TRUE(i->arguments[2]->Is<ast::BoolLiteralExpression>());
-    EXPECT_TRUE(i->arguments[3]->Is<ast::IdentifierExpression>());
+    auto* t = i->As<TemplatedIdentifier>();
+    ASSERT_NE(t, nullptr);
+    ASSERT_EQ(t->arguments.Length(), 4u);
+    EXPECT_TRUE(t->arguments[0]->Is<IntLiteralExpression>());
+    EXPECT_TRUE(t->arguments[1]->Is<BinaryExpression>());
+    EXPECT_TRUE(t->arguments[2]->Is<BoolLiteralExpression>());
+    EXPECT_TRUE(t->arguments[3]->Is<IdentifierExpression>());
 }
 
 TEST_F(TemplatedIdentifierTest, Creation_WithSource) {
     auto* i = Ident(Source{{20, 2}}, "ident", 1_a, Add("x", "y"), false, "x");
     EXPECT_EQ(i->symbol, Symbols().Get("ident"));
-    ASSERT_EQ(i->arguments.Length(), 4u);
-    EXPECT_TRUE(i->arguments[0]->Is<ast::IntLiteralExpression>());
-    EXPECT_TRUE(i->arguments[1]->Is<ast::BinaryExpression>());
-    EXPECT_TRUE(i->arguments[2]->Is<ast::BoolLiteralExpression>());
-    EXPECT_TRUE(i->arguments[3]->Is<ast::IdentifierExpression>());
+    auto* t = i->As<TemplatedIdentifier>();
+    ASSERT_NE(t, nullptr);
+    ASSERT_EQ(t->arguments.Length(), 4u);
+    EXPECT_TRUE(t->arguments[0]->Is<IntLiteralExpression>());
+    EXPECT_TRUE(t->arguments[1]->Is<BinaryExpression>());
+    EXPECT_TRUE(t->arguments[2]->Is<BoolLiteralExpression>());
+    EXPECT_TRUE(t->arguments[3]->Is<IdentifierExpression>());
 
     auto src = i->source;
     EXPECT_EQ(src.range.begin.line, 20u);
diff --git a/src/tint/program_builder.h b/src/tint/program_builder.h
index d52acfcef6..a1e3eed3e6 100644
--- a/src/tint/program_builder.h
+++ b/src/tint/program_builder.h
@@ -1496,23 +1496,24 @@ class ProgramBuilder {
 
     /// @param identifier the identifier symbol
     /// @param args the templated identifier arguments
-    /// @return an ast::TemplatedIdentifier with the given symbol and template arguments
+    /// @return an ast::Identifier with the given symbol and template arguments
     template <typename IDENTIFIER, typename... ARGS, typename = DisableIfSource<IDENTIFIER>>
-    const ast::TemplatedIdentifier* Ident(IDENTIFIER&& identifier, ARGS&&... args) {
+    const ast::Identifier* Ident(IDENTIFIER&& identifier, ARGS&&... args) {
         return Ident(source_, std::forward<IDENTIFIER>(identifier), std::forward<ARGS>(args)...);
     }
 
     /// @param source the source information
     /// @param identifier the identifier symbol
     /// @param args the templated identifier arguments
-    /// @return an ast::TemplatedIdentifier with the given symbol and template arguments
+    /// @return an ast::Identifier with the given symbol and template arguments
     template <typename IDENTIFIER, typename... ARGS>
-    const ast::TemplatedIdentifier* Ident(const Source& source,
-                                          IDENTIFIER&& identifier,
-                                          ARGS&&... args) {
+    const ast::Identifier* Ident(const Source& source, IDENTIFIER&& identifier, ARGS&&... args) {
+        auto arg_exprs = ExprList(std::forward<ARGS>(args)...);
+        if (arg_exprs.IsEmpty()) {
+            return create<ast::Identifier>(source, Sym(std::forward<IDENTIFIER>(identifier)));
+        }
         return create<ast::TemplatedIdentifier>(source, Sym(std::forward<IDENTIFIER>(identifier)),
-                                                ExprList(std::forward<ARGS>(args)...),
-                                                utils::Empty);
+                                                std::move(arg_exprs), utils::Empty);
     }
 
     /// @param expr the expression
diff --git a/src/tint/reader/wgsl/parser_impl_error_msg_test.cc b/src/tint/reader/wgsl/parser_impl_error_msg_test.cc
index 98001d110b..8213425c25 100644
--- a/src/tint/reader/wgsl/parser_impl_error_msg_test.cc
+++ b/src/tint/reader/wgsl/parser_impl_error_msg_test.cc
@@ -1111,5 +1111,13 @@ TEST_F(ParserImplErrorTest, InvalidUTF8) {
            "fn fu\xD0nc() {}\n");
 }
 
+TEST_F(ParserImplErrorTest, Bug_Chromium_1417465) {
+    EXPECT("var<workgroup> vec4_data: array<mat4x4<f@32>, 256>;",
+           R"(test.wgsl:1:41 error: expected ',' for template argument list
+var<workgroup> vec4_data: array<mat4x4<f@32>, 256>;
+                                        ^
+)");
+}
+
 }  // namespace
 }  // namespace tint::reader::wgsl