Add Diagnostics to Program, reader::[wgsl,spirv]::Parse()

By putting diagnostics into the program, we can hold all the diagnostic messages for parsing and type determination in one place.
This also means that we can simplify the public WGSL and SPIR-V Parser interfaces to a single function.

Change-Id: Ib6ab5fa180addd45c4aaf0c6b192d47182ffb50a
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/38920
Reviewed-by: dan sinclair <dsinclair@chromium.org>
Commit-Queue: Ben Clayton <bclayton@google.com>

fuzzers/tint_ast_clone_fuzzer.cc

@@ -54,6 +54,9 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
     return 0;
   }
   auto src = parser.program();
+  if (!src.IsValid()) {
+    return 0;
+  }
 
   // Clone the src program to dst
   tint::Program dst(src.Clone());

samples/main.cc

@@ -436,7 +436,7 @@ int main(int argc, const char** argv) {
   auto diag_printer = tint::diag::Printer::create(stderr, true);
   tint::diag::Formatter diag_formatter;
 
-  std::unique_ptr<tint::reader::Reader> reader;
+  std::unique_ptr<tint::Program> program;
   std::unique_ptr<tint::Source::File> source_file;
 #if TINT_BUILD_WGSL_READER
   if (options.input_filename.size() > 5 &&
@@ -448,7 +448,8 @@ int main(int argc, const char** argv) {
     }
     source_file = std::make_unique<tint::Source::File>(
         options.input_filename, std::string(data.begin(), data.end()));
-    reader = std::make_unique<tint::reader::wgsl::Parser>(source_file.get());
+    program = std::make_unique<tint::Program>(
+        tint::reader::wgsl::Parse(source_file.get()));
   }
 #endif  // TINT_BUILD_WGSL_READER
 
@@ -461,7 +462,7 @@ int main(int argc, const char** argv) {
     if (!ReadFile<uint32_t>(options.input_filename, &data)) {
       return 1;
     }
-    reader = std::make_unique<tint::reader::spirv::Parser>(data);
+    program = std::make_unique<tint::Program>(tint::reader::spirv::Parse(data));
   }
   // Handle SPIR-V assembly input, in files ending with .spvasm
   if (options.input_filename.size() > 7 &&
@@ -483,36 +484,31 @@ int main(int argc, const char** argv) {
                         SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS)) {
       return 1;
     }
-    reader = std::make_unique<tint::reader::spirv::Parser>(data);
+    program = std::make_unique<tint::Program>(tint::reader::spirv::Parse(data));
   }
 #endif  // TINT_BUILD_SPV_READER
 
-  if (!reader) {
+  if (!program) {
     std::cerr << "Failed to create reader for input file: "
               << options.input_filename << std::endl;
     return 1;
   }
-  if (!reader->Parse()) {
-    diag_formatter.format(reader->diagnostics(), diag_printer.get());
-    return 1;
-  }
-  auto program = reader->program();
-  if (!program.IsValid()) {
-    std::cerr << "Invalid program generated..." << std::endl;
+  if (!program->IsValid()) {
+    diag_formatter.format(program->Diagnostics(), diag_printer.get());
     return 1;
   }
 
-  auto diags = tint::TypeDeterminer::Run(&program);
+  auto diags = tint::TypeDeterminer::Run(program.get());
   if (diags.contains_errors()) {
     std::cerr << "Type Determination: ";
-    diag_formatter.format(reader->diagnostics(), diag_printer.get());
+    diag_formatter.format(diags, diag_printer.get());
     return 1;
   }
 
   if (options.dump_ast) {
-    auto ast_str = program.to_str();
+    auto ast_str = program->to_str();
     if (options.demangle) {
-      ast_str = tint::Demangler().Demangle(program.Symbols(), ast_str);
+      ast_str = tint::Demangler().Demangle(program->Symbols(), ast_str);
     }
     std::cout << std::endl << ast_str << std::endl;
   }
@@ -521,7 +517,7 @@ int main(int argc, const char** argv) {
   }
 
   tint::Validator v;
-  if (!v.Validate(&program)) {
+  if (!v.Validate(program.get())) {
     diag_formatter.format(v.diagnostics(), diag_printer.get());
     return 1;
   }
@@ -547,37 +543,37 @@ int main(int argc, const char** argv) {
     }
   }
 
-  auto out = transform_manager.Run(&program);
+  auto out = transform_manager.Run(program.get());
   if (out.diagnostics.contains_errors()) {
     diag_formatter.format(out.diagnostics, diag_printer.get());
     return 1;
   }
 
-  program = std::move(out.program);
+  *program = std::move(out.program);
 
   std::unique_ptr<tint::writer::Writer> writer;
 
 #if TINT_BUILD_SPV_WRITER
   if (options.format == Format::kSpirv || options.format == Format::kSpvAsm) {
-    writer = std::make_unique<tint::writer::spirv::Generator>(&program);
+    writer = std::make_unique<tint::writer::spirv::Generator>(program.get());
   }
 #endif  // TINT_BUILD_SPV_WRITER
 
 #if TINT_BUILD_WGSL_WRITER
   if (options.format == Format::kWgsl) {
-    writer = std::make_unique<tint::writer::wgsl::Generator>(&program);
+    writer = std::make_unique<tint::writer::wgsl::Generator>(program.get());
   }
 #endif  // TINT_BUILD_WGSL_WRITER
 
 #if TINT_BUILD_MSL_WRITER
   if (options.format == Format::kMsl) {
-    writer = std::make_unique<tint::writer::msl::Generator>(&program);
+    writer = std::make_unique<tint::writer::msl::Generator>(program.get());
   }
 #endif  // TINT_BUILD_MSL_WRITER
 
 #if TINT_BUILD_HLSL_WRITER
   if (options.format == Format::kHlsl) {
-    writer = std::make_unique<tint::writer::hlsl::Generator>(&program);
+    writer = std::make_unique<tint::writer::hlsl::Generator>(program.get());
   }
 #endif  // TINT_BUILD_HLSL_WRITER
 
@@ -642,7 +638,7 @@ int main(int argc, const char** argv) {
     auto* w = static_cast<tint::writer::Text*>(writer.get());
     auto output = w->result();
     if (options.demangle) {
-      output = tint::Demangler().Demangle(program.Symbols(), output);
+      output = tint::Demangler().Demangle(program->Symbols(), output);
     }
     if (!WriteFile(options.output_file, "w", output)) {
       return 1;

src/ast/module_clone_test.cc

@@ -75,7 +75,6 @@ fn f1(p0 : f32, p1 : i32) -> f32 {
   var l4 : S;
   var l5 : u32 = l4.m1[5];
   var l6 : ptr<private, u32>;
-  l6 = null;
   loop {
     l0 = (p1 + 2);
     if (((l0 % 4) == 0)) {
@@ -116,6 +115,8 @@ fn main() -> void {
   ASSERT_TRUE(parser.Parse()) << parser.error();
   auto src = parser.program();
 
+  ASSERT_TRUE(src.IsValid()) << diag::Formatter().format(src.Diagnostics());
+
   // Clone the src program to dst
   Program dst(src.Clone());
 

src/inspector/inspector_test.cc

@@ -615,6 +615,10 @@ class InspectorHelper : public ProgramBuilder {
       return *inspector_;
     }
     program_ = std::make_unique<Program>(std::move(*this));
+    [&]() {
+      ASSERT_TRUE(program_->IsValid())
+          << diag::Formatter().format(program_->Diagnostics());
+    }();
     inspector_ = std::make_unique<Inspector>(program_.get());
     return *inspector_;
   }

src/program.cc

@@ -20,6 +20,7 @@
 #include "src/ast/module.h"
 #include "src/clone_context.h"
 #include "src/program_builder.h"
+#include "src/type_determiner.h"
 
 namespace tint {
 
@@ -29,20 +30,31 @@ Program::Program(Program&& program)
     : types_(std::move(program.types_)),
       nodes_(std::move(program.nodes_)),
       ast_(std::move(program.ast_)),
-      symbols_(std::move(program.symbols_)) {
+      symbols_(std::move(program.symbols_)),
+      diagnostics_(std::move(program.diagnostics_)),
+      is_valid_(program.is_valid_) {
   program.AssertNotMoved();
   program.moved_ = true;
 }
 
-Program::Program(ProgramBuilder&& builder)
-    : types_(std::move(builder.Types())),
-      nodes_(std::move(builder.Nodes())),
-      ast_(nodes_.Create<ast::Module>(Source{},
-                                      builder.AST().ConstructedTypes(),
+Program::Program(ProgramBuilder&& builder) {
+  is_valid_ = builder.IsValid();  // must be called before the std::move()s
+
+  types_ = std::move(builder.Types());
+  nodes_ = std::move(builder.Nodes());
+  ast_ = nodes_.Create<ast::Module>(Source{}, builder.AST().ConstructedTypes(),
                                     builder.AST().Functions(),
-                                      builder.AST().GlobalVariables())),
-      symbols_(std::move(builder.Symbols())) {
+                                    builder.AST().GlobalVariables());
+  symbols_ = std::move(builder.Symbols());
+  diagnostics_ = std::move(builder.Diagnostics());
   builder.MarkAsMoved();
+
+  if (!is_valid_ && !diagnostics_.contains_errors()) {
+    // If the builder claims to be invalid, then we really should have an error
+    // message generated. If we find a situation where the program is not valid
+    // and there are no errors reported, add one here.
+    diagnostics_.add_error("invalid program generated");
+  }
 }
 
 Program::~Program() = default;
@@ -54,6 +66,7 @@ Program& Program::operator=(Program&& program) {
   nodes_ = std::move(program.nodes_);
   ast_ = std::move(program.ast_);
   symbols_ = std::move(program.symbols_);
+  is_valid_ = program.is_valid_;
   return *this;
 }
 
@@ -71,7 +84,7 @@ ProgramBuilder Program::CloneAsBuilder() const {
 
 bool Program::IsValid() const {
   AssertNotMoved();
-  return ast_->IsValid();
+  return is_valid_;
 }
 
 std::string Program::to_str() const {

src/program.h

@@ -18,6 +18,7 @@
 #include <string>
 
 #include "src/ast/function.h"
+#include "src/diagnostic/diagnostic.h"
 #include "src/symbol_table.h"
 #include "src/type/type_manager.h"
 
@@ -81,13 +82,20 @@ class Program {
     return symbols_;
   }
 
+  /// @returns a reference to the program's diagnostics
+  const diag::List& Diagnostics() const {
+    AssertNotMoved();
+    return diagnostics_;
+  }
+
   /// @return a deep copy of this program
   Program Clone() const;
 
   /// @return a deep copy of this Program, as a ProgramBuilder
   ProgramBuilder CloneAsBuilder() const;
 
-  /// @returns true if the program not missing information
+  /// @returns true if the program has no error diagnostics and is not missing
+  /// information
   bool IsValid() const;
 
   /// @returns a string describing this program.
@@ -103,6 +111,8 @@ class Program {
   ASTNodes nodes_;
   ast::Module* ast_;
   SymbolTable symbols_;
+  diag::List diagnostics_;
+  bool is_valid_ = true;
   bool moved_ = false;
 };
 

src/program_builder.cc

@@ -49,7 +49,7 @@ ProgramBuilder& ProgramBuilder::operator=(ProgramBuilder&& rhs) {
 }
 
 bool ProgramBuilder::IsValid() const {
-  return ast_->IsValid();
+  return !diagnostics_.contains_errors() && ast_->IsValid();
 }
 
 void ProgramBuilder::MarkAsMoved() {

src/program_builder.h

@@ -35,6 +35,7 @@
 #include "src/ast/type_constructor_expression.h"
 #include "src/ast/uint_literal.h"
 #include "src/ast/variable.h"
+#include "src/diagnostic/diagnostic.h"
 #include "src/program.h"
 #include "src/symbol_table.h"
 #include "src/type/alias_type.h"
@@ -118,7 +119,14 @@ class ProgramBuilder {
     return symbols_;
   }
 
-  /// @returns true if the program not missing information
+  /// @returns a reference to the program's diagnostics
+  diag::List& Diagnostics() {
+    AssertNotMoved();
+    return diagnostics_;
+  }
+
+  /// @returns true if the program has no error diagnostics and is not missing
+  /// information
   bool IsValid() const;
 
   /// creates a new ast::Node owned by the Module. When the Module is
@@ -837,6 +845,7 @@ class ProgramBuilder {
   ASTNodes nodes_;
   ast::Module* ast_;
   SymbolTable symbols_;
+  diag::List diagnostics_;
 
   /// The source to use when creating AST nodes without providing a Source as
   /// the first argument.

src/program_test.cc

@@ -128,5 +128,15 @@ TEST_F(ProgramTest, IsValid_Invalid_Function) {
   EXPECT_FALSE(program.IsValid());
 }
 
+TEST_F(ProgramTest, IsValid_GeneratesError) {
+  AST().AddGlobalVariable(nullptr);
+
+  Program program(std::move(*this));
+  EXPECT_FALSE(program.IsValid());
+  EXPECT_EQ(program.Diagnostics().count(), 1u);
+  EXPECT_EQ(program.Diagnostics().error_count(), 1u);
+  EXPECT_EQ(program.Diagnostics().begin()->message,
+            "invalid program generated");
+}
 }  // namespace
 }  // namespace tint

src/reader/spirv/parser.cc

@@ -43,6 +43,17 @@ Program Parser::program() {
   return impl_->program();
 }
 
+Program Parse(const std::vector<uint32_t>& input) {
+  ParserImpl parser(input);
+  bool parsed = parser.Parse();
+  ProgramBuilder builder = std::move(parser.builder());
+  if (!parsed) {
+    // TODO(bclayton): Migrate spirv::ParserImpl to using diagnostics.
+    builder.Diagnostics().add_error(parser.error());
+  }
+  return Program(std::move(builder));
+}
+
 }  // namespace spirv
 }  // namespace reader
 }  // namespace tint

src/reader/spirv/parser.h

@@ -28,6 +28,7 @@ namespace spirv {
 class ParserImpl;
 
 /// Parser for SPIR-V source data
+/// [DEPRECATED] - Use Parse()
 class Parser : public Reader {
  public:
   /// Creates a new parser
@@ -46,9 +47,16 @@ class Parser : public Reader {
 
  private:
   std::unique_ptr<ParserImpl> impl_;
-  Program program_;
 };
 
+/// Parses the SPIR-V source data, returning the parsed program.
+/// If the source data fails to parse then the returned
+/// `program.Diagnostics.contains_errors()` will be true, and the
+/// `program.Diagnostics()` will describe the error.
+/// @param input the source data
+/// @returns the parsed program
+Program Parse(const std::vector<uint32_t>& input);
+
 }  // namespace spirv
 }  // namespace reader
 }  // namespace tint

src/reader/spirv/parser_test.cc

@@ -26,13 +26,21 @@ namespace {
 
 using ParserTest = testing::Test;
 
-TEST_F(ParserTest, Uint32VecEmpty) {
+TEST_F(ParserTest, Uint32VecEmptyOld) {
   std::vector<uint32_t> data;
   Parser p(data);
   EXPECT_FALSE(p.Parse());
   // TODO(dneto): What message?
 }
 
+TEST_F(ParserTest, DataEmpty) {
+  std::vector<uint32_t> data;
+  auto program = Parse(data);
+  auto errs = diag::Formatter().format(program.Diagnostics());
+  ASSERT_FALSE(program.IsValid()) << errs;
+  EXPECT_EQ(errs, "error: line:0: Invalid SPIR-V magic number.\n");
+}
+
 // TODO(dneto): uint32 vec, valid SPIR-V
 // TODO(dneto): uint32 vec, invalid SPIR-V
 

src/reader/wgsl/parser.cc

@@ -39,6 +39,16 @@ Program Parser::program() {
   return impl_->program();
 }
 
+Program Parse(Source::File const* file) {
+  ParserImpl parser(file);
+  parser.Parse();
+  ProgramBuilder builder = std::move(parser.builder());
+  // TODO(bclayton): Remove ParserImpl::diagnostics() and put all diagnostic
+  // into the builder.
+  builder.Diagnostics().add(parser.diagnostics());
+  return Program(std::move(builder));
+}
+
 }  // namespace wgsl
 }  // namespace reader
 }  // namespace tint

src/reader/wgsl/parser.h

@@ -28,6 +28,7 @@ namespace wgsl {
 class ParserImpl;
 
 /// Parser for WGSL source data
+/// [DEPRECATED] - Use Parse()
 class Parser : public Reader {
  public:
   /// Creates a new parser from the given file.
@@ -47,6 +48,14 @@ class Parser : public Reader {
   std::unique_ptr<ParserImpl> impl_;
 };
 
+/// Parses the WGSL source, returning the parsed program.
+/// If the source fails to parse then the returned
+/// `program.Diagnostics.contains_errors()` will be true, and the
+/// `program.Diagnostics()` will describe the error.
+/// @param file the source file
+/// @returns the parsed program
+Program Parse(Source::File const* file);
+
 }  // namespace wgsl
 }  // namespace reader
 }  // namespace tint

src/reader/wgsl/parser_test.cc

@@ -25,13 +25,13 @@ namespace {
 
 using ParserTest = testing::Test;
 
-TEST_F(ParserTest, Empty) {
+TEST_F(ParserTest, EmptyOld) {
   Source::File file("test.wgsl", "");
   Parser p(&file);
   ASSERT_TRUE(p.Parse()) << p.error();
 }
 
-TEST_F(ParserTest, Parses) {
+TEST_F(ParserTest, ParsesOld) {
   Source::File file("test.wgsl", R"(
 [[location(0)]] var<out> gl_FragColor : vec4<f32>;
 
@@ -48,7 +48,7 @@ fn main() -> void {
   ASSERT_EQ(1u, program.AST().GlobalVariables().size());
 }
 
-TEST_F(ParserTest, HandlesError) {
+TEST_F(ParserTest, HandlesErrorOld) {
   Source::File file("test.wgsl", R"(
 fn main() ->  {  // missing return type
   return;
@@ -60,6 +60,48 @@ fn main() ->  {  // missing return type
   EXPECT_EQ(p.error(), "2:15: unable to determine function return type");
 }
 
+TEST_F(ParserTest, Empty) {
+  Source::File file("test.wgsl", "");
+  auto program = Parse(&file);
+  auto errs = diag::Formatter().format(program.Diagnostics());
+  ASSERT_TRUE(program.IsValid()) << errs;
+}
+
+TEST_F(ParserTest, Parses) {
+  Source::File file("test.wgsl", R"(
+[[location(0)]] var<out> gl_FragColor : vec4<f32>;
+
+[[stage(vertex)]]
+fn main() -> void {
+  gl_FragColor = vec4<f32>(.4, .2, .3, 1);
+}
+)");
+  Parser p(&file);
+  auto program = Parse(&file);
+  auto errs = diag::Formatter().format(program.Diagnostics());
+  ASSERT_TRUE(program.IsValid()) << errs;
+
+  ASSERT_EQ(1u, program.AST().Functions().size());
+  ASSERT_EQ(1u, program.AST().GlobalVariables().size());
+}
+
+TEST_F(ParserTest, HandlesError) {
+  Source::File file("test.wgsl", R"(
+fn main() ->  {  // missing return type
+  return;
+})");
+
+  auto program = Parse(&file);
+  auto errs = diag::Formatter().format(program.Diagnostics());
+  ASSERT_FALSE(program.IsValid()) << errs;
+  EXPECT_EQ(errs,
+            R"(test.wgsl:2:15 error: unable to determine function return type
+fn main() ->  {  // missing return type
+              ^
+
+)");
+}
+
 }  // namespace
 }  // namespace wgsl
 }  // namespace reader

src/transform/test_helper.h

@@ -52,6 +52,10 @@ class TransformTest : public testing::Test {
     style.print_newline_at_end = false;
 
     auto program = parser.program();
+    if (!program.IsValid()) {
+      return diag::Formatter(style).format(program.Diagnostics());
+    }
+
     {
       auto diagnostics = TypeDeterminer::Run(&program);
       if (diagnostics.contains_errors()) {