From 368fc0063c3820e0d99103bbbb2dd80400611f39 Mon Sep 17 00:00:00 2001
From: Jon Meow <46229924+jonmeow@users.noreply.github.com>
Date: Tue, 20 Jul 2021 12:04:45 -0700
Subject: [PATCH] Handle invalid chars better, and do small cleanups around
 error output. (#657)

This adds a test for invalid characters (that would've failed before, because the printed char isn't escaped). Not sure if there's a good way to test the PrintDiagnostic code, as it appears to occur on bison parser errors, which I'm just not sure how to trigger.
---
 executable_semantics/BUILD                           |  1 +
 executable_semantics/syntax/lexer.lpp                | 12 ++++++++++--
 .../syntax/parse_and_lex_context.cpp                 |  9 ++++-----
 executable_semantics/syntax/parser.ypp               | 12 +++++-------
 executable_semantics/testdata/invalid_char.carbon    |  1 +
 executable_semantics/testdata/invalid_char.golden    |  2 ++
 6 files changed, 23 insertions(+), 14 deletions(-)
 create mode 100644 executable_semantics/testdata/invalid_char.carbon
 create mode 100644 executable_semantics/testdata/invalid_char.golden

diff --git a/executable_semantics/BUILD b/executable_semantics/BUILD
index 45e5d9b510f9..7809520bf4c6 100644
--- a/executable_semantics/BUILD
+++ b/executable_semantics/BUILD
@@ -54,6 +54,7 @@ EXAMPLES = [
     "if1",
     "if2",
     "if3",
+    "invalid_char",
     "match_any_int",
     "match_int_default",
     "match_int",
diff --git a/executable_semantics/syntax/lexer.lpp b/executable_semantics/syntax/lexer.lpp
index 1ac3e5cebf64..d70bfe2e0ec3 100644
--- a/executable_semantics/syntax/lexer.lpp
+++ b/executable_semantics/syntax/lexer.lpp
@@ -7,7 +7,10 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 %{
 #include <cstdlib>
 #include <iostream>
+
+#include "executable_semantics/tracing_flag.h"
 #include "executable_semantics/syntax/parse_and_lex_context.h"
+#include "llvm/ADT/StringExtras.h"
 %}
 
 /* Turn off legacy bits we don't need */
@@ -208,8 +211,13 @@ operator and its operand, leading to three more cases:
 }
 
 . {
-  std::cerr << context.current_token_position << ": invalid character '"
-            << yytext[0] << "' in source file." << std::endl;
+  if (Carbon::tracing_output) {
+    // Print a newline because tracing prints an incomplete line
+    // "Reading a token: ".
+    std::cerr << std::endl;
+  }
+  std::cerr << context.current_token_position << ": invalid character '\\x"
+            << llvm::toHex(llvm::StringRef(yytext, 1)) << "' in source file." << std::endl;
   std::exit(1);
 }
 
diff --git a/executable_semantics/syntax/parse_and_lex_context.cpp b/executable_semantics/syntax/parse_and_lex_context.cpp
index 150abd0b6e56..5cea9f8a3b88 100644
--- a/executable_semantics/syntax/parse_and_lex_context.cpp
+++ b/executable_semantics/syntax/parse_and_lex_context.cpp
@@ -7,15 +7,14 @@
 #include <cstring>
 #include <iostream>
 
-#include "executable_semantics/tracing_flag.h"
-
 // Writes a syntax error diagnostic, containing message, for the input file at
 // the given line, to standard error.
 auto Carbon::ParseAndLexContext::PrintDiagnostic(const std::string& message,
                                                  int line_num) -> void {
+  // TODO: Do we really want this to be fatal?  It makes the comment and the
+  // name a lie, and renders some of the other yyparse() result propagation code
+  // moot.
   std::cerr << input_file_name << ":" << line_num << ": " << message
             << std::endl;
-  exit(-1);  // TODO: do we really want this here?  It makes the comment and the
-             // name a lie, and renders some of the other yyparse() result
-             // propagation code moot.
+  exit(-1);
 }
diff --git a/executable_semantics/syntax/parser.ypp b/executable_semantics/syntax/parser.ypp
index 82ac9ddeaf43..58756de95fed 100644
--- a/executable_semantics/syntax/parser.ypp
+++ b/executable_semantics/syntax/parser.ypp
@@ -61,7 +61,7 @@
 
 #include "executable_semantics/syntax/syntax_helpers.h"
 #include "executable_semantics/syntax/parse_and_lex_context.h"
-}
+}  // %code top
 
 %code requires {
 #include <optional>
@@ -73,21 +73,19 @@
 
 namespace Carbon {
 class ParseAndLexContext;
-}
+}  // namespace Carbon
 
-}
+}  // %code requires
 
 %code {
 
 extern int yylineno;
 
-void yy::parser::error(
-    const location_type&, const std::string& message)
-{
+void yy::parser::error(const location_type&, const std::string& message) {
   context.PrintDiagnostic(message, yylineno);
 }
 
-}
+}  // %code
 
 %token <int> integer_literal
 %token <char*> identifier
diff --git a/executable_semantics/testdata/invalid_char.carbon b/executable_semantics/testdata/invalid_char.carbon
new file mode 100644
index 000000000000..081c298f731f
--- /dev/null
+++ b/executable_semantics/testdata/invalid_char.carbon
@@ -0,0 +1 @@
+þ
diff --git a/executable_semantics/testdata/invalid_char.golden b/executable_semantics/testdata/invalid_char.golden
new file mode 100644
index 000000000000..f10e7562fe31
--- /dev/null
+++ b/executable_semantics/testdata/invalid_char.golden
@@ -0,0 +1,2 @@
+1.1: invalid character '\xFE' in source file.
+EXIT CODE: 1