From aa5f23e1caa2ecc750efc282713df44c663c2390 Mon Sep 17 00:00:00 2001
From: Ben Clayton <bclayton@google.com>
Date: Thu, 19 Nov 2020 14:19:31 +0000
Subject: [PATCH] reader/wgsl: Improve errors for statements outside functions

Fixes: tint:328
Change-Id: I29c35605c2cf546080e28eaa9d983595b4a8d977
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/33401
Auto-Submit: Ben Clayton <bclayton@google.com>
Commit-Queue: dan sinclair <dsinclair@chromium.org>
Reviewed-by: dan sinclair <dsinclair@chromium.org>
---
 src/reader/wgsl/parser_impl.cc                | 49 +++++++++++++++----
 src/reader/wgsl/parser_impl.h                 | 10 ++++
 src/reader/wgsl/parser_impl_error_msg_test.cc | 14 ++++++
 3 files changed, 63 insertions(+), 10 deletions(-)

diff --git a/src/reader/wgsl/parser_impl.cc b/src/reader/wgsl/parser_impl.cc
index 40b37e599a..cecf9b837a 100644
--- a/src/reader/wgsl/parser_impl.cc
+++ b/src/reader/wgsl/parser_impl.cc
@@ -185,11 +185,13 @@ ParserImpl::Failure::Errored ParserImpl::add_error(const Token& t,
 
 ParserImpl::Failure::Errored ParserImpl::add_error(const Source& source,
                                                    const std::string& err) {
-  diag::Diagnostic diagnostic;
-  diagnostic.severity = diag::Severity::Error;
-  diagnostic.message = err;
-  diagnostic.source = source;
-  diags_.add(std::move(diagnostic));
+  if (silence_errors_ == 0) {
+    diag::Diagnostic diagnostic;
+    diagnostic.severity = diag::Severity::Error;
+    diagnostic.message = err;
+    diagnostic.source = source;
+    diags_.add(std::move(diagnostic));
+  }
   return Failure::kErrored;
 }
 
@@ -329,12 +331,31 @@ Expect<bool> ParserImpl::expect_global_decl() {
   if (errored)
     return Failure::kErrored;
 
-  if (decos.value.size() > 0) {
-    add_error(next(), "expected declaration after decorations");
-  } else {
-    add_error(next(), "unexpected token");
+  // Invalid syntax found - try and determine the best error message
+
+  // We have decorations parsed, but nothing to consume them?
+  if (decos.value.size() > 0)
+    return add_error(next(), "expected declaration after decorations");
+
+  // We have a statement outside of a function?
+  auto t = peek();
+  auto stat = without_error([&] { return statement(); });
+  if (stat.matched) {
+    // Attempt to jump to the next '}' - the function might have just been
+    // missing an opening line.
+    sync_to(Token::Type::kBraceRight, true);
+    return add_error(t, "statement found outside of function body");
   }
-  return Failure::kErrored;
+  if (!stat.errored) {
+    // No match, no error - the parser might not have progressed.
+    // Ensure we always make _some_ forward progress.
+    next();
+  }
+
+  // Exhausted all attempts to make sense of where we're at.
+  // Spew a generic error.
+
+  return add_error(t, "unexpected token");
 }
 
 // global_variable_decl
@@ -3001,6 +3022,14 @@ bool ParserImpl::is_sync_token(const Token& t) const {
   return false;
 }
 
+template <typename F, typename T>
+T ParserImpl::without_error(F&& body) {
+  silence_errors_++;
+  auto result = body();
+  silence_errors_--;
+  return result;
+}
+
 }  // namespace wgsl
 }  // namespace reader
 }  // namespace tint
diff --git a/src/reader/wgsl/parser_impl.h b/src/reader/wgsl/parser_impl.h
index 09c5f5da1d..6604102f66 100644
--- a/src/reader/wgsl/parser_impl.h
+++ b/src/reader/wgsl/parser_impl.h
@@ -711,6 +711,15 @@ class ParserImpl {
   /// @see sync().
   bool is_sync_token(const Token& t) const;
 
+  /// without_error() calls the function |func| muting any grammatical errors
+  /// found while executing the function. This can be used as a best-effort to
+  /// produce a meaningful error message when the parser is out of sync.
+  /// @param func a function or lambda with the signature: `Expect<Result>()` or
+  /// `Maybe<Result>()`.
+  /// @return the value returned by |func|.
+  template <typename F, typename T = ReturnType<F>>
+  T without_error(F&& func);
+
   /// Downcasts all the decorations in |list| to the type |T|, raising a parser
   /// error if any of the decorations aren't of the type |T|.
   template <typename T>
@@ -749,6 +758,7 @@ class ParserImpl {
   std::deque<Token> token_queue_;
   bool synchronized_ = true;
   std::vector<Token::Type> sync_tokens_;
+  int silence_errors_ = 0;
   std::unordered_map<std::string, ast::type::Type*> registered_constructs_;
   ast::Module module_;
 };
diff --git a/src/reader/wgsl/parser_impl_error_msg_test.cc b/src/reader/wgsl/parser_impl_error_msg_test.cc
index 7e075a56c4..8a5c15d1bb 100644
--- a/src/reader/wgsl/parser_impl_error_msg_test.cc
+++ b/src/reader/wgsl/parser_impl_error_msg_test.cc
@@ -449,6 +449,20 @@ TEST_F(ParserImplErrorTest, FunctionDeclMissingRBrace) {
          "                ^\n");
 }
 
+TEST_F(ParserImplErrorTest, FunctionMissingOpenLine) {
+  EXPECT(R"(const bar : vec2<f32> = vec2<f32>(1., 2.);
+  var a : f32 = bar[0];
+  return;
+})",
+         "test.wgsl:2:17 error: unknown constructed type 'bar'\n"
+         "  var a : f32 = bar[0];\n"
+         "                ^^^\n"
+         "\n"
+         "test.wgsl:3:3 error: statement found outside of function body\n"
+         "  return;\n"
+         "  ^^^^^^\n");
+}
+
 TEST_F(ParserImplErrorTest, GlobalDeclConstInvalidIdentifier) {
   EXPECT("const ^ : i32 = 1;",
          "test.wgsl:1:7 error: expected identifier for constant declaration\n"