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>
diff --git a/fuzzers/tint_ast_clone_fuzzer.cc b/fuzzers/tint_ast_clone_fuzzer.cc
index 0e4cee6..65cdf9c 100644
--- a/fuzzers/tint_ast_clone_fuzzer.cc
+++ b/fuzzers/tint_ast_clone_fuzzer.cc
@@ -54,6 +54,9 @@
return 0;
}
auto src = parser.program();
+ if (!src.IsValid()) {
+ return 0;
+ }
// Clone the src program to dst
tint::Program dst(src.Clone());
diff --git a/samples/main.cc b/samples/main.cc
index 2e5264d..c90fe95 100644
--- a/samples/main.cc
+++ b/samples/main.cc
@@ -436,7 +436,7 @@
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 @@
}
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 @@
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 @@
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 @@
}
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 @@
}
}
- 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 @@
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;
diff --git a/src/ast/module_clone_test.cc b/src/ast/module_clone_test.cc
index a3bc10f..d2d72a1 100644
--- a/src/ast/module_clone_test.cc
+++ b/src/ast/module_clone_test.cc
@@ -75,7 +75,6 @@
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 @@
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());
diff --git a/src/inspector/inspector_test.cc b/src/inspector/inspector_test.cc
index 8f53a60..cd5e026 100644
--- a/src/inspector/inspector_test.cc
+++ b/src/inspector/inspector_test.cc
@@ -615,6 +615,10 @@
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_;
}
diff --git a/src/program.cc b/src/program.cc
index be8ceec..d8393a7 100644
--- a/src/program.cc
+++ b/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 @@
: 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(),
- builder.AST().Functions(),
- builder.AST().GlobalVariables())),
- symbols_(std::move(builder.Symbols())) {
+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());
+ 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 @@
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 @@
bool Program::IsValid() const {
AssertNotMoved();
- return ast_->IsValid();
+ return is_valid_;
}
std::string Program::to_str() const {
diff --git a/src/program.h b/src/program.h
index 09b5ec7..bddc73d 100644
--- a/src/program.h
+++ b/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 @@
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 @@
ASTNodes nodes_;
ast::Module* ast_;
SymbolTable symbols_;
+ diag::List diagnostics_;
+ bool is_valid_ = true;
bool moved_ = false;
};
diff --git a/src/program_builder.cc b/src/program_builder.cc
index ccc053d..af61240 100644
--- a/src/program_builder.cc
+++ b/src/program_builder.cc
@@ -49,7 +49,7 @@
}
bool ProgramBuilder::IsValid() const {
- return ast_->IsValid();
+ return !diagnostics_.contains_errors() && ast_->IsValid();
}
void ProgramBuilder::MarkAsMoved() {
diff --git a/src/program_builder.h b/src/program_builder.h
index 7e3d602..9d71236 100644
--- a/src/program_builder.h
+++ b/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 @@
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 @@
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.
diff --git a/src/program_test.cc b/src/program_test.cc
index a352b0b..333fab7 100644
--- a/src/program_test.cc
+++ b/src/program_test.cc
@@ -128,5 +128,15 @@
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
diff --git a/src/reader/spirv/parser.cc b/src/reader/spirv/parser.cc
index 51cd2f0..75b6475 100644
--- a/src/reader/spirv/parser.cc
+++ b/src/reader/spirv/parser.cc
@@ -43,6 +43,17 @@
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
diff --git a/src/reader/spirv/parser.h b/src/reader/spirv/parser.h
index 711b50d..cf9e53a 100644
--- a/src/reader/spirv/parser.h
+++ b/src/reader/spirv/parser.h
@@ -28,6 +28,7 @@
class ParserImpl;
/// Parser for SPIR-V source data
+/// [DEPRECATED] - Use Parse()
class Parser : public Reader {
public:
/// Creates a new parser
@@ -46,9 +47,16 @@
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
diff --git a/src/reader/spirv/parser_test.cc b/src/reader/spirv/parser_test.cc
index 962354c..8c4d7c1 100644
--- a/src/reader/spirv/parser_test.cc
+++ b/src/reader/spirv/parser_test.cc
@@ -26,13 +26,21 @@
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
diff --git a/src/reader/wgsl/parser.cc b/src/reader/wgsl/parser.cc
index aec5b68..4296e4d 100644
--- a/src/reader/wgsl/parser.cc
+++ b/src/reader/wgsl/parser.cc
@@ -39,6 +39,16 @@
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
diff --git a/src/reader/wgsl/parser.h b/src/reader/wgsl/parser.h
index ddc8217..1960396 100644
--- a/src/reader/wgsl/parser.h
+++ b/src/reader/wgsl/parser.h
@@ -28,6 +28,7 @@
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 @@
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
diff --git a/src/reader/wgsl/parser_test.cc b/src/reader/wgsl/parser_test.cc
index 4925cfd..030652e 100644
--- a/src/reader/wgsl/parser_test.cc
+++ b/src/reader/wgsl/parser_test.cc
@@ -25,13 +25,13 @@
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 @@
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 @@
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
diff --git a/src/transform/test_helper.h b/src/transform/test_helper.h
index a0f64bd..a141791 100644
--- a/src/transform/test_helper.h
+++ b/src/transform/test_helper.h
@@ -52,6 +52,10 @@
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()) {
