Clarify "it" in error messages. Also add a to_string method on Err, allowing us to write proper unit tests on the actual error string returned. Change-Id: I0b7c3ecfa4220bd4a29ac43e8c0928be6a6a6964 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/24460 Reviewed-by: Takuto Ikuta <tikuta@google.com> Reviewed-by: Richard Wang <richardwa@google.com> Commit-Queue: Matt Stark <msta@google.com>
diff --git a/src/gn/err.cc b/src/gn/err.cc index e72d86a..ca61e7c 100644 --- a/src/gn/err.cc +++ b/src/gn/err.cc
@@ -92,12 +92,37 @@ } } +} // namespace + +struct ErrOutput { + virtual ~ErrOutput() = default; + virtual void Write(std::string_view text, + TextDecoration dec = DECORATION_NONE) = 0; +}; + +namespace { + +struct StdoutErrOutput : public ErrOutput { + StdoutErrOutput(bool is_fatal) : is_fatal(is_fatal) {} + void Write(std::string_view text, TextDecoration dec) override { + OutputErrString(is_fatal, text, dec); + } + bool is_fatal; +}; + +struct StringErrOutput : public ErrOutput { + void Write(std::string_view text, TextDecoration dec) override { + out.append(text); + } + std::string out; +}; + // The line length is used to clip the maximum length of the markers we'll // make if the error spans more than one line (like unterminated literals). -void OutputHighlighedPosition(const Location& location, - const Err::RangeList& ranges, - size_t line_length, - bool is_fatal) { +void OutputHighlightedPosition(const Location& location, + const Err::RangeList& ranges, + size_t line_length, + ErrOutput& output) { // Make a buffer of the line in spaces. std::string highlight; highlight.resize(line_length); @@ -119,7 +144,7 @@ highlight.resize(highlight.size() - 1); highlight += "\n"; - OutputErrString(is_fatal, highlight, DECORATION_BLUE); + output.Write(highlight, DECORATION_BLUE); } } // namespace @@ -209,11 +234,23 @@ return false; } } + } + StdoutErrOutput output(is_fatal); + InternalFormat(output, is_sub_err, is_fatal); + return true; +} + +void Err::InternalFormat(ErrOutput& output, + bool is_sub_err, + bool is_fatal) const { + DCHECK(info_); + + if (!is_sub_err) { if (is_fatal) - OutputString("ERROR ", DECORATION_RED); + output.Write("ERROR ", DECORATION_RED); else - OutputLogString("WARNING ", DECORATION_MAGENTA); + output.Write("WARNING ", DECORATION_MAGENTA); } // File name and location. @@ -235,27 +272,32 @@ std::string colon; if (!loc_str.empty() || !toolchain_str.empty()) colon = ": "; - OutputErrString(is_fatal, - loc_str + toolchain_str + colon + info_->message + "\n"); + output.Write(loc_str + toolchain_str + colon + info_->message + "\n"); // Quoted line. if (input_file) { std::string line = GetNthLine(input_file->contents(), info_->location.line_number()); if (!base::ContainsOnlyChars(line, base::kWhitespaceASCII)) { - OutputErrString(is_fatal, line + "\n", DECORATION_DIM); - OutputHighlighedPosition(info_->location, info_->ranges, line.size(), - is_fatal); + output.Write(line + "\n", DECORATION_DIM); + OutputHighlightedPosition(info_->location, info_->ranges, line.size(), + output); } } // Optional help text. if (!info_->help_text.empty()) - OutputErrString(is_fatal, info_->help_text + "\n"); + output.Write(info_->help_text + "\n"); // Sub errors. for (const auto& sub_err : info_->sub_errs) - sub_err.InternalPrintToStdout(true, is_fatal); + sub_err.InternalFormat(output, true, is_fatal); +} - return true; +std::string Err::to_string() const { + if (!has_error()) + return std::string(); + StringErrOutput output; + InternalFormat(output, false, true); + return std::move(output.out); }
diff --git a/src/gn/err.h b/src/gn/err.h index c67da5d..098ea95 100644 --- a/src/gn/err.h +++ b/src/gn/err.h
@@ -15,6 +15,7 @@ class ParseNode; class Value; +struct ErrOutput; // Result of doing some operation. Check has_error() to see if an error // occurred. @@ -105,6 +106,10 @@ // newlines or separators. bool PrintToStdout() const; + // Converts the error to a string, formatted as it would be if calling + // PrintToStdout with text decoration disabled. + std::string to_string() const; + // Prints to standard out but uses a "WARNING" messaging instead of the // normal "ERROR" messaging. This is a property of the printing system rather // than of the Err class because there is no expectation that code calling a @@ -121,6 +126,7 @@ private: bool InternalPrintToStdout(bool is_sub_err, bool is_fatal) const; + void InternalFormat(ErrOutput& output, bool is_sub_err, bool is_fatal) const; std::unique_ptr<ErrInfo> info_; // Non-null indicates error. };
diff --git a/src/gn/import_manager.cc b/src/gn/import_manager.cc index 69f44b4..c422c91 100644 --- a/src/gn/import_manager.cc +++ b/src/gn/import_manager.cc
@@ -42,7 +42,8 @@ if (err->has_error()) { // If there was an error, append the caller location so the error message // displays a why the file was imported (esp. useful for failed asserts). - err->AppendSubErr(Err(node_for_err, "whence it was imported.")); + err->AppendSubErr( + Err(node_for_err, "whence " + file.value() + " was imported.")); return nullptr; } scope->ClearProcessingImport();
diff --git a/src/gn/template.cc b/src/gn/template.cc index a01cd8c..5a1c936 100644 --- a/src/gn/template.cc +++ b/src/gn/template.cc
@@ -106,7 +106,8 @@ if (err->has_error()) { // If there was an error, append the caller location so the error message // displays a stack trace of how it got here. - err->AppendSubErr(Err(invocation, "whence it was called.")); + err->AppendSubErr( + Err(invocation, "whence " + template_name + " was called.")); return Value(); } @@ -124,7 +125,8 @@ if (!invoker_value->scope_value()->CheckForUnusedVars(err)) { // If there was an error, append the caller location so the error message // displays a stack trace of how it got here. - err->AppendSubErr(Err(invocation, "whence it was called.")); + err->AppendSubErr( + Err(invocation, "whence " + template_name + " was called.")); return Value(); } } @@ -133,7 +135,8 @@ if (!template_scope.CheckForUnusedVars(err)) { // If there was an error, append the caller location so the error message // displays a stack trace of how it got here. - err->AppendSubErr(Err(invocation, "whence it was called.")); + err->AppendSubErr( + Err(invocation, "whence " + template_name + " was called.")); return Value(); }
diff --git a/src/gn/template_unittest.cc b/src/gn/template_unittest.cc index 58dbf66..b4fdf1a 100644 --- a/src/gn/template_unittest.cc +++ b/src/gn/template_unittest.cc
@@ -91,3 +91,26 @@ input.parsed()->Execute(setup.scope(), &err); ASSERT_SUCCESS(input); } + +TEST(Template, ErrorStackTrace) { + TestWithScope setup; + TestParseInput input( + "template(\"my_template\") {\n" + " print(invoker.undefined_var)\n" + "}\n" + "my_template(\"lala\") {\n" + "}"); + ASSERT_SUCCESS(input); + + Err err; + input.parsed()->Execute(setup.scope(), &err); + EXPECT_TRUE(err.has_error()); + EXPECT_EQ(err.to_string(), + "ERROR at //test:2:17: No value named \"undefined_var\" in scope " + "\"invoker\"\n" + " print(invoker.undefined_var)\n" + " ^------------\n" + "See //test:4:1: whence my_template was called.\n" + "my_template(\"lala\") {\n" + "^--------------------\n"); +}