From d1981c765ae3016a891c235116b0462725904edb Mon Sep 17 00:00:00 2001 From: Pawel Sagan Date: Mon, 25 Jan 2021 22:29:26 +0100 Subject: [PATCH 1/8] option to show line with diagnostic and a marker Adding: - show_diagnostic_context option for linter to print vulnerable code snippet with test - shell script tests Signed-off-by: Pawel Sagan --- common/analysis/BUILD | 1 + common/analysis/file_analyzer.cc | 1 + common/analysis/lint_rule_status.cc | 28 +++++++++++++-- common/analysis/lint_rule_status.h | 11 ++++++ common/analysis/lint_rule_status_test.cc | 43 +++++++++++++++++++++--- verilog/analysis/verilog_linter.cc | 19 +++++++---- verilog/analysis/verilog_linter.h | 5 +-- verilog/analysis/verilog_linter_test.cc | 42 ++++++++++++++++------- verilog/tools/lint/lint_tool_test.sh | 9 +++++ verilog/tools/lint/verilog_lint.cc | 7 +++- 10 files changed, 137 insertions(+), 29 deletions(-) diff --git a/common/analysis/BUILD b/common/analysis/BUILD index 83823611e..d40b53078 100644 --- a/common/analysis/BUILD +++ b/common/analysis/BUILD @@ -128,6 +128,7 @@ cc_library( "//common/text:text_structure", "//common/text:token_info", "//common/text:token_stream_view", + "//common/util:spacer", "@com_google_absl//absl/status", "@com_google_absl//absl/strings", ], diff --git a/common/analysis/file_analyzer.cc b/common/analysis/file_analyzer.cc index 7db16b7fb..69704dca6 100644 --- a/common/analysis/file_analyzer.cc +++ b/common/analysis/file_analyzer.cc @@ -30,6 +30,7 @@ #include "common/text/text_structure.h" #include "common/text/token_info.h" #include "common/text/token_stream_view.h" +#include "common/util/spacer.h" namespace verible { diff --git a/common/analysis/lint_rule_status.cc b/common/analysis/lint_rule_status.cc index 7b859a977..26eaa64af 100644 --- a/common/analysis/lint_rule_status.cc +++ b/common/analysis/lint_rule_status.cc @@ -28,6 +28,7 @@ #include "common/text/syntax_tree_context.h" #include "common/text/token_info.h" #include "common/text/tree_utils.h" +#include "common/util/spacer.h" namespace verible { @@ -71,9 +72,8 @@ struct LintViolationWithStatus { } }; -void LintStatusFormatter::FormatLintRuleStatuses( - std::ostream* stream, const std::vector& statuses, - absl::string_view base, absl::string_view path) const { +static std::set PrepareViolations( + const std::vector& statuses) { std::set violations; // TODO(fangism): rewrite as a linear time merge of pre-ordered sub-sequences @@ -82,11 +82,33 @@ void LintStatusFormatter::FormatLintRuleStatuses( violations.insert(LintViolationWithStatus(&violation, &status)); } } + return violations; +} + +void LintStatusFormatter::FormatLintRuleStatuses( + std::ostream* stream, const std::vector& statuses, + absl::string_view base, absl::string_view path) const { + auto violations = PrepareViolations(statuses); + for (auto violation : violations) { + FormatViolation(stream, *violation.violation, base, path, + violation.status->url, violation.status->lint_rule_name); + *stream << std::endl; + } +} + +void LintStatusFormatter::FormatLintRuleStatuses( + std::ostream* stream, const std::vector& statuses, + absl::string_view base, absl::string_view path, + const std::vector& lines) const { + auto violations = PrepareViolations(statuses); for (auto violation : violations) { FormatViolation(stream, *violation.violation, base, path, violation.status->url, violation.status->lint_rule_name); *stream << std::endl; + auto cursor = line_column_map_(violation.violation->token.left(base)); + *stream << lines[cursor.line] << std::endl; + *stream << verible::Spacer(cursor.column) << "^" << std::endl; } } diff --git a/common/analysis/lint_rule_status.h b/common/analysis/lint_rule_status.h index 10d01ec0b..8556ace32 100644 --- a/common/analysis/lint_rule_status.h +++ b/common/analysis/lint_rule_status.h @@ -134,6 +134,17 @@ class LintStatusFormatter { absl::string_view base, absl::string_view path) const; + // Formats, sorts and outputs status to stream with additional vulnerable code + // line printed when enabled. The violations contained in the statuses are + // sorted by their occurrence in the code and are not grouped by the status + // object. Path is the file path of original file. This is needed because it + // is not contained in status. Base is the string_view of the entire contents, + // used only for byte offset calculation. + void FormatLintRuleStatuses( + std::ostream* stream, const std::vector& statuses, + absl::string_view base, absl::string_view path, + const std::vector& lines) const; + // Formats and outputs violation on stream. // Path is file path of original file and url is a link to the ratified rule // that is being violated. diff --git a/common/analysis/lint_rule_status_test.cc b/common/analysis/lint_rule_status_test.cc index b172f1088..e37440155 100644 --- a/common/analysis/lint_rule_status_test.cc +++ b/common/analysis/lint_rule_status_test.cc @@ -144,7 +144,7 @@ TEST(LintRuleStatusFormatterTest, NoOutput) { RunLintStatusTest(test); } -void RunLintStatusesTest(const LintStatusTest& test) { +void RunLintStatusesTest(const LintStatusTest& test, bool show_context) { // Dummy tree so we have something for test cases to point at SymbolPtr root = Node(); @@ -172,8 +172,21 @@ void RunLintStatusesTest(const LintStatusTest& test) { std::ostringstream ss; LintStatusFormatter formatter(test.text); - formatter.FormatLintRuleStatuses(&ss, statuses, test.text, test.path); - auto result_parts = absl::StrSplit(ss.str(), '\n'); + const std::vector lines; + if (!show_context) { + formatter.FormatLintRuleStatuses(&ss, statuses, test.text, test.path); + } else { + formatter.FormatLintRuleStatuses(&ss, statuses, test.text, test.path, + absl::StrSplit(test.text, '\n')); + std::cout << ss.str() << std::endl; + } + std::vector result_parts; + if (!show_context) + result_parts = absl::StrSplit(ss.str(), '\n'); + else { + result_parts = absl::StrSplit(ss.str(), "^\n"); + } + auto part_iterator = result_parts.begin(); for (const auto& violation_test : test.violations) { @@ -200,7 +213,29 @@ TEST(LintRuleStatusFormatterTest, MultipleStatusesSimpleOutput) { {"reason2", TokenInfo(dont_care_tag, text.substr(21, 4)), "some/path/to/somewhere.fvg:2:4: reason2 http://foobar [test-rule]"}}}; - RunLintStatusesTest(test); + RunLintStatusesTest(test, false); +} + +TEST(LintRuleStatusFormatterTestWithContext, MultipleStatusesSimpleOutput) { + SymbolPtr root = Node(); + static const int dont_care_tag = 0; + constexpr absl::string_view text( + "This is some code\n" + "That you are looking at right now\n" + "It is nice code, make no mistake\n" + "Very nice"); + LintStatusTest test = { + "test-rule", + "http://foobar", + "some/path/to/somewhere.fvg", + text, + {{"reason1", TokenInfo(dont_care_tag, text.substr(0, 5)), + "some/path/to/somewhere.fvg:1:1: reason1 http://foobar " + "[test-rule]\nThis is some code\n"}, + {"reason2", TokenInfo(dont_care_tag, text.substr(21, 4)), + "some/path/to/somewhere.fvg:2:4: reason2 http://foobar " + "[test-rule]\nThat you are looking at right now\n "}}}; + RunLintStatusesTest(test, true); } } // namespace diff --git a/verilog/analysis/verilog_linter.cc b/verilog/analysis/verilog_linter.cc index fe7092c0a..f07814407 100644 --- a/verilog/analysis/verilog_linter.cc +++ b/verilog/analysis/verilog_linter.cc @@ -83,7 +83,7 @@ using verible::TokenInfo; // 2..: other fatal issues such as file not found. int LintOneFile(std::ostream* stream, absl::string_view filename, const LinterConfiguration& config, bool check_syntax, - bool parse_fatal, bool lint_fatal) { + bool parse_fatal, bool lint_fatal, bool show_context) { std::string content; const absl::Status content_status = verible::file::GetContents(filename, &content); @@ -115,8 +115,9 @@ int LintOneFile(std::ostream* stream, absl::string_view filename, // Analyze the parsed structure for lint violations. std::ostringstream lint_stream; - const absl::Status lint_status = VerilogLintTextStructure( - &lint_stream, std::string(filename), content, config, analyzer->Data()); + const absl::Status lint_status = + VerilogLintTextStructure(&lint_stream, std::string(filename), content, + config, analyzer->Data(), show_context); if (!lint_status.ok()) { // Something went wrong with running the lint analysis itself. LOG(ERROR) << "Fatal error: " << lint_status.message(); @@ -271,7 +272,8 @@ absl::Status VerilogLintTextStructure(std::ostream* stream, const std::string& filename, const std::string& contents, const LinterConfiguration& config, - const TextStructureView& text_structure) { + const TextStructureView& text_structure, + bool show_context) { // Create the linter, add rules, and run it. VerilogLinter linter; const absl::Status configuration_status = linter.Configure(config, filename); @@ -296,8 +298,13 @@ absl::Status VerilogLintTextStructure(std::ostream* stream, VLOG(1) << "Lint Violations (" << total_violations << "): " << std::endl; // Output results to stream using formatter. verible::LintStatusFormatter formatter(contents); - formatter.FormatLintRuleStatuses(stream, linter_statuses, text_base, - filename); + if (!show_context) { + formatter.FormatLintRuleStatuses(stream, linter_statuses, text_base, + filename); + } else { + formatter.FormatLintRuleStatuses(stream, linter_statuses, text_base, + filename, text_structure.Lines()); + } } return absl::OkStatus(); } diff --git a/verilog/analysis/verilog_linter.h b/verilog/analysis/verilog_linter.h index 773aa4a97..2dd6b35c5 100644 --- a/verilog/analysis/verilog_linter.h +++ b/verilog/analysis/verilog_linter.h @@ -47,7 +47,7 @@ namespace verilog { // errors were found (syntax, lint), and anything else is a fatal error. int LintOneFile(std::ostream* stream, absl::string_view filename, const LinterConfiguration& config, bool check_syntax, - bool parse_fatal, bool lint_fatal); + bool parse_fatal, bool lint_fatal, bool show_context); // VerilogLinter analyzes a TextStructureView of Verilog source code. // This uses syntax-tree based analyses and lexical token-stream analyses. @@ -109,13 +109,14 @@ absl::Status AppendLinterConfigurationFromFile( // and its sole purpose is to provide a translation from byte-offset to // line-column in diagnostics. // text_structure: contains the syntax tree that will be lint-analyzed. +// show_context: print additional line with vulnerable code // // Returns: // absl::Status that reflects whether linter linter ran successfully. absl::Status VerilogLintTextStructure( std::ostream* stream, const std::string& filename, const std::string& contents, const LinterConfiguration& config, - const verible::TextStructureView& text_structure); + const verible::TextStructureView& text_structure, bool show_context); // Prints the rule, description and default_enabled. absl::Status PrintRuleInfo(std::ostream*, diff --git a/verilog/analysis/verilog_linter_test.cc b/verilog/analysis/verilog_linter_test.cc index 7fd1403ff..6f5832636 100644 --- a/verilog/analysis/verilog_linter_test.cc +++ b/verilog/analysis/verilog_linter_test.cc @@ -66,8 +66,8 @@ class LintOneFileTest : public DefaultLinterConfigTestFixture, // Tests that nonexistent file is handled as a fatal error. TEST_F(LintOneFileTest, FileNotFound) { std::ostringstream output; - const int exit_code = - LintOneFile(&output, "FileNotFound.sv", config_, true, false, false); + const int exit_code = LintOneFile(&output, "FileNotFound.sv", config_, true, + false, false, false); EXPECT_EQ(exit_code, 2); } @@ -81,11 +81,20 @@ TEST_F(LintOneFileTest, LintCleanFiles) { }; for (const auto test_code : kTestCases) { const ScopedTestFile temp_file(testing::TempDir(), test_code); - std::ostringstream output; - const int exit_code = - LintOneFile(&output, temp_file.filename(), config_, true, false, false); - EXPECT_EQ(exit_code, 0); - EXPECT_TRUE(output.str().empty()); // silence + { + std::ostringstream output; + const int exit_code = LintOneFile(&output, temp_file.filename(), config_, + true, false, false, false); + EXPECT_EQ(exit_code, 0); + EXPECT_TRUE(output.str().empty()); // silence + } + { // enable additional error context printing + std::ostringstream output; + const int exit_code = LintOneFile(&output, temp_file.filename(), config_, + true, false, false, true); + EXPECT_EQ(exit_code, 0); + EXPECT_TRUE(output.str().empty()); // silence + } } } @@ -101,21 +110,28 @@ TEST_F(LintOneFileTest, SyntaxError) { { // continue even with syntax error std::ostringstream output; const int exit_code = LintOneFile(&output, temp_file.filename(), config_, - true, false, false); + true, false, false, false); + EXPECT_EQ(exit_code, 0); + EXPECT_FALSE(output.str().empty()); + } + { // continue even with syntax error with additional error context + std::ostringstream output; + const int exit_code = LintOneFile(&output, temp_file.filename(), config_, + true, false, false, true); EXPECT_EQ(exit_code, 0); EXPECT_FALSE(output.str().empty()); } { // abort on syntax error std::ostringstream output; const int exit_code = LintOneFile(&output, temp_file.filename(), config_, - true, true, false); + true, true, false, false); EXPECT_EQ(exit_code, 1); EXPECT_FALSE(output.str().empty()); } { // ignore syntax error std::ostringstream output; const int exit_code = LintOneFile(&output, temp_file.filename(), config_, - false, false, false); + false, false, false, false); EXPECT_EQ(exit_code, 0); EXPECT_TRUE(output.str().empty()); // silence } @@ -133,14 +149,14 @@ TEST_F(LintOneFileTest, LintError) { { // continue even with lint error std::ostringstream output; const int exit_code = LintOneFile(&output, temp_file.filename(), config_, - true, false, false); + true, false, false, false); EXPECT_EQ(exit_code, 0) << "output:\n" << output.str(); EXPECT_FALSE(output.str().empty()); } { // abort on lint error std::ostringstream output; const int exit_code = LintOneFile(&output, temp_file.filename(), config_, - true, false, true); + true, false, true, false); EXPECT_EQ(exit_code, 1) << "output:\n" << output.str(); EXPECT_FALSE(output.str().empty()); } @@ -172,7 +188,7 @@ class VerilogLinterTest : public DefaultLinterConfigTestFixture, // lint success, so as long as we have a syntax tree (even if there // are errors), run the lint checks. const absl::Status lint_status = VerilogLintTextStructure( - &diagnostics, filename, content, config_, analyzer->Data()); + &diagnostics, filename, content, config_, analyzer->Data(), false); return {lint_status, diagnostics.str()}; } }; diff --git a/verilog/tools/lint/lint_tool_test.sh b/verilog/tools/lint/lint_tool_test.sh index 8036b9d61..c23708057 100755 --- a/verilog/tools/lint/lint_tool_test.sh +++ b/verilog/tools/lint/lint_tool_test.sh @@ -96,6 +96,15 @@ status="$?" exit 1 } +echo "=== Test --show_diagnostic_context" +"$lint_tool" "$TEST_FILE" --show_diagnostic_context > /dev/null 2> "${MY_OUTPUT_FILE}.err" + +status="$?" +[[ $status == 1 ]] || { + echo "Expected exit code 1, but got $status" + exit 1 +} + echo "=== Test --parse_fatal" "$lint_tool" "$TEST_FILE" --parse_fatal > /dev/null 2> "${MY_OUTPUT_FILE}.err" diff --git a/verilog/tools/lint/verilog_lint.cc b/verilog/tools/lint/verilog_lint.cc index 59f591ece..af4cb62e4 100644 --- a/verilog/tools/lint/verilog_lint.cc +++ b/verilog/tools/lint/verilog_lint.cc @@ -50,6 +50,10 @@ ABSL_FLAG( "Markdown and exit immediately. Intended for the output to be written " "to a snippet of Markdown."); +ABSL_FLAG(bool, show_diagnostic_context, false, + "prints an additional " + "line on which the diagnostic was found," + "followed by a line with a position marker"); // LINT.ThenChange(README.md) using verilog::LinterConfiguration; @@ -83,7 +87,8 @@ int main(int argc, char** argv) { const int lint_status = verilog::LintOneFile( &std::cout, filename, config, // absl::GetFlag(FLAGS_check_syntax), absl::GetFlag(FLAGS_parse_fatal), - absl::GetFlag(FLAGS_lint_fatal)); + absl::GetFlag(FLAGS_lint_fatal), + absl::GetFlag(FLAGS_show_diagnostic_context)); exit_status = std::max(lint_status, exit_status); } // for each file From 602e259a64c241facc20e744dd6cdbcc6e5d2974 Mon Sep 17 00:00:00 2001 From: Pawel Sagan Date: Fri, 5 Mar 2021 15:14:40 +0100 Subject: [PATCH 2/8] removing duplicated functions --- common/analysis/lint_rule_status.cc | 32 +++++++----------------- common/analysis/lint_rule_status.h | 17 +++---------- common/analysis/lint_rule_status_test.cc | 2 +- verilog/analysis/verilog_linter.cc | 2 +- 4 files changed, 15 insertions(+), 38 deletions(-) diff --git a/common/analysis/lint_rule_status.cc b/common/analysis/lint_rule_status.cc index 26eaa64af..6616d4b60 100644 --- a/common/analysis/lint_rule_status.cc +++ b/common/analysis/lint_rule_status.cc @@ -16,6 +16,7 @@ #include #include +#include #include #include #include @@ -72,8 +73,10 @@ struct LintViolationWithStatus { } }; -static std::set PrepareViolations( - const std::vector& statuses) { +void LintStatusFormatter::FormatLintRuleStatuses( + std::ostream* stream, const std::vector& statuses, + absl::string_view base, absl::string_view path, + const std::vector& lines) const { std::set violations; // TODO(fangism): rewrite as a linear time merge of pre-ordered sub-sequences @@ -82,33 +85,16 @@ static std::set PrepareViolations( violations.insert(LintViolationWithStatus(&violation, &status)); } } - return violations; -} - -void LintStatusFormatter::FormatLintRuleStatuses( - std::ostream* stream, const std::vector& statuses, - absl::string_view base, absl::string_view path) const { - auto violations = PrepareViolations(statuses); - for (auto violation : violations) { - FormatViolation(stream, *violation.violation, base, path, - violation.status->url, violation.status->lint_rule_name); - *stream << std::endl; - } -} - -void LintStatusFormatter::FormatLintRuleStatuses( - std::ostream* stream, const std::vector& statuses, - absl::string_view base, absl::string_view path, - const std::vector& lines) const { - auto violations = PrepareViolations(statuses); for (auto violation : violations) { FormatViolation(stream, *violation.violation, base, path, violation.status->url, violation.status->lint_rule_name); *stream << std::endl; auto cursor = line_column_map_(violation.violation->token.left(base)); - *stream << lines[cursor.line] << std::endl; - *stream << verible::Spacer(cursor.column) << "^" << std::endl; + if (cursor.line < lines.size()) { + *stream << lines[cursor.line] << std::endl; + *stream << verible::Spacer(cursor.column) << "^" << std::endl; + } } } diff --git a/common/analysis/lint_rule_status.h b/common/analysis/lint_rule_status.h index 8556ace32..7a5b96772 100644 --- a/common/analysis/lint_rule_status.h +++ b/common/analysis/lint_rule_status.h @@ -122,7 +122,8 @@ class LintStatusFormatter { absl::string_view base, absl::string_view path) const; - // Formats, sorts and outputs status to stream. + // Formats, sorts and outputs status to stream with additional vulnerable code + // line printed when enabled. // The violations contained in the statuses are sorted by their occurrence // in the code and are not grouped by the status object. // Path is the file path of original file. This is needed because it is not @@ -132,18 +133,8 @@ class LintStatusFormatter { void FormatLintRuleStatuses(std::ostream* stream, const std::vector& statuses, absl::string_view base, - absl::string_view path) const; - - // Formats, sorts and outputs status to stream with additional vulnerable code - // line printed when enabled. The violations contained in the statuses are - // sorted by their occurrence in the code and are not grouped by the status - // object. Path is the file path of original file. This is needed because it - // is not contained in status. Base is the string_view of the entire contents, - // used only for byte offset calculation. - void FormatLintRuleStatuses( - std::ostream* stream, const std::vector& statuses, - absl::string_view base, absl::string_view path, - const std::vector& lines) const; + absl::string_view path, + const std::vector& lines) const; // Formats and outputs violation on stream. // Path is file path of original file and url is a link to the ratified rule diff --git a/common/analysis/lint_rule_status_test.cc b/common/analysis/lint_rule_status_test.cc index e37440155..b84e66378 100644 --- a/common/analysis/lint_rule_status_test.cc +++ b/common/analysis/lint_rule_status_test.cc @@ -174,7 +174,7 @@ void RunLintStatusesTest(const LintStatusTest& test, bool show_context) { LintStatusFormatter formatter(test.text); const std::vector lines; if (!show_context) { - formatter.FormatLintRuleStatuses(&ss, statuses, test.text, test.path); + formatter.FormatLintRuleStatuses(&ss, statuses, test.text, test.path, {}); } else { formatter.FormatLintRuleStatuses(&ss, statuses, test.text, test.path, absl::StrSplit(test.text, '\n')); diff --git a/verilog/analysis/verilog_linter.cc b/verilog/analysis/verilog_linter.cc index f07814407..4f18867bf 100644 --- a/verilog/analysis/verilog_linter.cc +++ b/verilog/analysis/verilog_linter.cc @@ -300,7 +300,7 @@ absl::Status VerilogLintTextStructure(std::ostream* stream, verible::LintStatusFormatter formatter(contents); if (!show_context) { formatter.FormatLintRuleStatuses(stream, linter_statuses, text_base, - filename); + filename, {}); } else { formatter.FormatLintRuleStatuses(stream, linter_statuses, text_base, filename, text_structure.Lines()); From 664f3b55d74f9a4a81964b9e3af1a61726e94dec Mon Sep 17 00:00:00 2001 From: Pawel Sagan Date: Fri, 5 Mar 2021 18:24:26 +0100 Subject: [PATCH 3/8] adding additional context to verible-verilog-syntax --- common/analysis/file_analyzer.cc | 15 ++++++++++++--- common/analysis/file_analyzer.h | 4 ++-- common/analysis/file_analyzer_test.cc | 12 ++++++++---- common/analysis/lint_rule_status.cc | 2 +- verilog/analysis/verilog_analyzer_test.cc | 3 ++- verilog/analysis/verilog_linter.cc | 3 ++- verilog/analysis/verilog_linter_test.cc | 3 ++- verilog/formatting/formatter.cc | 3 ++- verilog/formatting/tree_unwrapper_test.cc | 3 ++- verilog/tools/syntax/verilog_syntax.cc | 7 +++++++ 10 files changed, 40 insertions(+), 15 deletions(-) diff --git a/common/analysis/file_analyzer.cc b/common/analysis/file_analyzer.cc index 69704dca6..7218c5a0e 100644 --- a/common/analysis/file_analyzer.cc +++ b/common/analysis/file_analyzer.cc @@ -136,15 +136,24 @@ std::vector FileAnalyzer::TokenErrorMessages() const { } std::string FileAnalyzer::LinterTokenErrorMessage( - const RejectedToken& error_token) const { + const RejectedToken& error_token, bool diagnostic_context) const { const LineColumnMap& line_column_map = Data().GetLineColumnMap(); const absl::string_view base_text = Data().Contents(); + std::ostringstream output_stream; output_stream << filename_ << ':'; if (!error_token.token_info.isEOF()) { const auto left = line_column_map(error_token.token_info.left(base_text)); output_stream << left << ": " << error_token.phase << " error, rejected \"" << error_token.token_info.text() << "\" (syntax-error)."; + if(diagnostic_context) + { + const auto& lines = Data().Lines(); + if (left.line < static_cast(lines.size())) { + output_stream << "\n" << lines[left.line] << std::endl; + output_stream << verible::Spacer(left.column) << "^"; + } + } } else { const int file_size = base_text.length(); const auto end = line_column_map(file_size); @@ -158,11 +167,11 @@ std::string FileAnalyzer::LinterTokenErrorMessage( return output_stream.str(); } -std::vector FileAnalyzer::LinterTokenErrorMessages() const { +std::vector FileAnalyzer::LinterTokenErrorMessages(bool diagnostic_context) const { std::vector messages; messages.reserve(rejected_tokens_.size()); for (const auto& rejected_token : rejected_tokens_) { - messages.push_back(LinterTokenErrorMessage(rejected_token)); + messages.push_back(LinterTokenErrorMessage(rejected_token, diagnostic_context)); } return messages; } diff --git a/common/analysis/file_analyzer.h b/common/analysis/file_analyzer.h index a263ac4e2..faedfa57b 100644 --- a/common/analysis/file_analyzer.h +++ b/common/analysis/file_analyzer.h @@ -94,9 +94,9 @@ class FileAnalyzer : public TextStructure { std::vector TokenErrorMessages() const; // Diagnostic message for rejected tokens for linter. - std::string LinterTokenErrorMessage(const RejectedToken&) const; + std::string LinterTokenErrorMessage(const RejectedToken&, bool) const; - std::vector LinterTokenErrorMessages() const; + std::vector LinterTokenErrorMessages(bool) const; const std::vector& GetRejectedTokens() const { return rejected_tokens_; diff --git a/common/analysis/file_analyzer_test.cc b/common/analysis/file_analyzer_test.cc index 4e4787d13..879398607 100644 --- a/common/analysis/file_analyzer_test.cc +++ b/common/analysis/file_analyzer_test.cc @@ -63,8 +63,9 @@ TEST(FileAnalyzerTest, TokenErrorMessageSameLine) { EXPECT_EQ(message, "token: \"w0rld\" at 2:5-9"); } { + constexpr bool with_diagnostic_contex = false; const auto message = analyzer.LinterTokenErrorMessage( - {error_token, AnalysisPhase::kParsePhase}); + {error_token, AnalysisPhase::kParsePhase}, with_diagnostic_contex); EXPECT_TRUE(absl::StrContains( message, "hello.txt:2:5: syntax error, rejected \"w0rld\"")); } @@ -80,8 +81,9 @@ TEST(FileAnalyzerTest, TokenErrorMessageOneChar) { EXPECT_EQ(message, "token: \",\" at 1:6"); } { + constexpr bool with_diagnostic_contex = false; const auto message = analyzer.LinterTokenErrorMessage( - {error_token, AnalysisPhase::kParsePhase}); + {error_token, AnalysisPhase::kParsePhase}, with_diagnostic_contex); EXPECT_TRUE(absl::StrContains( message, "hello.txt:1:6: syntax error, rejected \",\"")); } @@ -97,8 +99,9 @@ TEST(FileAnalyzerTest, TokenErrorMessageDifferentLine) { EXPECT_EQ(message, "token: \"world\nbye\" at 1:8-2:3"); } { + constexpr bool with_diagnostic_contex = false; const auto message = analyzer.LinterTokenErrorMessage( - {error_token, AnalysisPhase::kParsePhase}); + {error_token, AnalysisPhase::kParsePhase}, with_diagnostic_contex); EXPECT_TRUE(absl::StrContains( message, "hello.txt:1:8: syntax error, rejected \"world\nbye\"")); } @@ -114,8 +117,9 @@ TEST(FileAnalyzerTest, TokenErrorMessageEOF) { EXPECT_EQ(message, "token: <> at 3:1"); } { + constexpr bool with_diagnostic_contex = false; const auto message = analyzer.LinterTokenErrorMessage( - {error_token, AnalysisPhase::kParsePhase}); + {error_token, AnalysisPhase::kParsePhase}, with_diagnostic_contex); EXPECT_TRUE(absl::StrContains( message, "unbalanced.txt:3:1: syntax error (unexpected EOF)")); } diff --git a/common/analysis/lint_rule_status.cc b/common/analysis/lint_rule_status.cc index 6616d4b60..7323a368d 100644 --- a/common/analysis/lint_rule_status.cc +++ b/common/analysis/lint_rule_status.cc @@ -91,7 +91,7 @@ void LintStatusFormatter::FormatLintRuleStatuses( violation.status->url, violation.status->lint_rule_name); *stream << std::endl; auto cursor = line_column_map_(violation.violation->token.left(base)); - if (cursor.line < lines.size()) { + if (cursor.line < static_cast(lines.size())) { *stream << lines[cursor.line] << std::endl; *stream << verible::Spacer(cursor.column) << "^" << std::endl; } diff --git a/verilog/analysis/verilog_analyzer_test.cc b/verilog/analysis/verilog_analyzer_test.cc index 84896e76a..225ca376b 100644 --- a/verilog/analysis/verilog_analyzer_test.cc +++ b/verilog/analysis/verilog_analyzer_test.cc @@ -75,8 +75,9 @@ bool TreeContainsToken(const ConcreteSyntaxTree& tree, const TokenInfo& token) { void DiagnosticMessagesContainFilename(const VerilogAnalyzer& analyzer, absl::string_view filename) { + constexpr bool with_diagnostic_contex = false; const std::vector syntax_error_messages( - analyzer.LinterTokenErrorMessages()); + analyzer.LinterTokenErrorMessages(with_diagnostic_contex)); for (const auto& message : syntax_error_messages) { EXPECT_TRUE(absl::StrContains(message, filename)); } diff --git a/verilog/analysis/verilog_linter.cc b/verilog/analysis/verilog_linter.cc index 4f18867bf..a688cbcc7 100644 --- a/verilog/analysis/verilog_linter.cc +++ b/verilog/analysis/verilog_linter.cc @@ -100,8 +100,9 @@ int LintOneFile(std::ostream* stream, absl::string_view filename, const auto lex_status = ABSL_DIE_IF_NULL(analyzer)->LexStatus(); const auto parse_status = analyzer->ParseStatus(); if (!lex_status.ok() || !parse_status.ok()) { + constexpr bool with_diagnostic_context = false; const std::vector syntax_error_messages( - analyzer->LinterTokenErrorMessages()); + analyzer->LinterTokenErrorMessages(with_diagnostic_context)); for (const auto& message : syntax_error_messages) { *stream << message << std::endl; } diff --git a/verilog/analysis/verilog_linter_test.cc b/verilog/analysis/verilog_linter_test.cc index 6f5832636..09f6fbf56 100644 --- a/verilog/analysis/verilog_linter_test.cc +++ b/verilog/analysis/verilog_linter_test.cc @@ -178,8 +178,9 @@ class VerilogLinterTest : public DefaultLinterConfigTestFixture, const absl::Status status = ABSL_DIE_IF_NULL(analyzer)->Analyze(); std::ostringstream diagnostics; if (!status.ok()) { + constexpr bool with_diagnostic_contex = false; const std::vector syntax_error_messages( - analyzer->LinterTokenErrorMessages()); + analyzer->LinterTokenErrorMessages(with_diagnostic_contex)); for (const auto& message : syntax_error_messages) { diagnostics << message << std::endl; } diff --git a/verilog/formatting/formatter.cc b/verilog/formatting/formatter.cc index 91d1060d5..52187580f 100644 --- a/verilog/formatting/formatter.cc +++ b/verilog/formatting/formatter.cc @@ -205,8 +205,9 @@ Status FormatVerilog(absl::string_view text, absl::string_view filename, const auto parse_status = analyzer->ParseStatus(); if (!lex_status.ok() || !parse_status.ok()) { std::ostringstream errstream; + constexpr bool with_diagnostic_context = false; const std::vector syntax_error_messages( - analyzer->LinterTokenErrorMessages()); + analyzer->LinterTokenErrorMessages(with_diagnostic_context)); for (const auto& message : syntax_error_messages) { errstream << message << std::endl; } diff --git a/verilog/formatting/tree_unwrapper_test.cc b/verilog/formatting/tree_unwrapper_test.cc index d8c51bc9b..21f66d612 100644 --- a/verilog/formatting/tree_unwrapper_test.cc +++ b/verilog/formatting/tree_unwrapper_test.cc @@ -239,8 +239,9 @@ class TreeUnwrapperTest : public ::testing::Test { // Since source code is required to be valid, this error-handling is just // to help debug the test case construction if (!status.ok()) { + constexpr bool with_diagnostic_contex = false; const std::vector syntax_error_messages( - analyzer_->LinterTokenErrorMessages()); + analyzer_->LinterTokenErrorMessages(with_diagnostic_contex)); for (const auto& message : syntax_error_messages) { std::cout << message << std::endl; } diff --git a/verilog/tools/syntax/verilog_syntax.cc b/verilog/tools/syntax/verilog_syntax.cc index ba2f7d32e..99cf1e2f1 100644 --- a/verilog/tools/syntax/verilog_syntax.cc +++ b/verilog/tools/syntax/verilog_syntax.cc @@ -103,6 +103,11 @@ ABSL_FLAG( bool, verifytree, false, "Verifies that all tokens are parsed into tree, prints unmatched tokens"); +ABSL_FLAG(bool, show_diagnostic_context, false, + "prints an additional " + "line on which the diagnostic was found," + "followed by a line with a position marker"); + using verible::ConcreteSyntaxTree; using verible::ParserVerifier; using verible::TextStructureView; @@ -151,6 +156,8 @@ static int AnalyzeOneFile(absl::string_view content, absl::string_view filename, const auto parse_status = analyzer->ParseStatus(); if (!lex_status.ok() || !parse_status.ok()) { + const std::vector syntax_error_messages( + analyzer->LinterTokenErrorMessages(absl::GetFlag(FLAGS_show_diagnostic_context))); const int error_limit = absl::GetFlag(FLAGS_error_limit); int error_count = 0; if (!absl::GetFlag(FLAGS_export_json)) { From cc9c22c5280067e12d5883aa7e1952b263274b0f Mon Sep 17 00:00:00 2001 From: Pawel Sagan Date: Fri, 5 Mar 2021 19:20:14 +0100 Subject: [PATCH 4/8] adding tests for tokenErrorMessages function --- common/analysis/file_analyzer_test.cc | 79 +++++++++++++++++++++++ verilog/analysis/verilog_analyzer_test.cc | 30 +++++---- verilog/analysis/verilog_linter_test.cc | 9 ++- 3 files changed, 106 insertions(+), 12 deletions(-) diff --git a/common/analysis/file_analyzer_test.cc b/common/analysis/file_analyzer_test.cc index 879398607..0c138651f 100644 --- a/common/analysis/file_analyzer_test.cc +++ b/common/analysis/file_analyzer_test.cc @@ -71,6 +71,26 @@ TEST(FileAnalyzerTest, TokenErrorMessageSameLine) { } } +// Verify that an error token on one line is reported correctly. +TEST(FileAnalyzerTest, TokenErrorMessageSameLineWithContext) { + const std::string text("hello, world\nbye w0rld\n"); + FakeFileAnalyzer analyzer(text, "hello.txt"); + const TokenInfo error_token(1, analyzer.Data().Contents().substr(17, 5)); + { + const auto message = analyzer.TokenErrorMessage(error_token); + EXPECT_EQ(message, "token: \"w0rld\" at 2:5-9"); + } + { + constexpr bool with_diagnostic_contex = true; + const auto message = analyzer.LinterTokenErrorMessage( + {error_token, AnalysisPhase::kParsePhase}, with_diagnostic_contex); + EXPECT_TRUE( + absl::StrContains(message, + "hello.txt:2:5: syntax error, rejected \"w0rld\" " + "(syntax-error).\nbye w0rld\n ^")); + } +} + // Verify that an error token on one character is reported correctly. TEST(FileAnalyzerTest, TokenErrorMessageOneChar) { const std::string text("hello, world\nbye w0rld\n"); @@ -89,6 +109,25 @@ TEST(FileAnalyzerTest, TokenErrorMessageOneChar) { } } +// Verify that an error token on one character is reported correctly. +TEST(FileAnalyzerTest, TokenErrorMessageOneCharWithContext) { + const std::string text("hello, world\nbye w0rld\n"); + FakeFileAnalyzer analyzer(text, "hello.txt"); + const TokenInfo error_token(1, analyzer.Data().Contents().substr(5, 1)); + { + const auto message = analyzer.TokenErrorMessage(error_token); + EXPECT_EQ(message, "token: \",\" at 1:6"); + } + { + constexpr bool with_diagnostic_contex = true; + const auto message = analyzer.LinterTokenErrorMessage( + {error_token, AnalysisPhase::kParsePhase}, with_diagnostic_contex); + EXPECT_TRUE(absl::StrContains(message, + "hello.txt:1:6: syntax error, rejected \",\" " + "(syntax-error).\nhello, world\n ^")); + } +} + // Verify that an error token spanning multiple lines is reported correctly. TEST(FileAnalyzerTest, TokenErrorMessageDifferentLine) { const std::string text("hello, world\nbye w0rld\n"); @@ -106,6 +145,27 @@ TEST(FileAnalyzerTest, TokenErrorMessageDifferentLine) { message, "hello.txt:1:8: syntax error, rejected \"world\nbye\"")); } } + +// Verify that an error token spanning multiple lines is reported correctly. +TEST(FileAnalyzerTest, TokenErrorMessageDifferentLineWithContext) { + const std::string text("hello, world\nbye w0rld\n"); + FakeFileAnalyzer analyzer(text, "hello.txt"); + const TokenInfo error_token(1, analyzer.Data().Contents().substr(7, 9)); + { + const auto message = analyzer.TokenErrorMessage(error_token); + EXPECT_EQ(message, "token: \"world\nbye\" at 1:8-2:3"); + } + { + constexpr bool with_diagnostic_contex = true; + const auto message = analyzer.LinterTokenErrorMessage( + {error_token, AnalysisPhase::kParsePhase}, with_diagnostic_contex); + EXPECT_TRUE(absl::StrContains( + message, + "hello.txt:1:8: syntax error, rejected \"world\nbye\" " + "(syntax-error).\nhello, world\n ^")); + } +} + // // Verify that an error at EOF reported correctly. TEST(FileAnalyzerTest, TokenErrorMessageEOF) { @@ -125,5 +185,24 @@ TEST(FileAnalyzerTest, TokenErrorMessageEOF) { } } +// +// Verify that an error at EOF reported correctly. +TEST(FileAnalyzerTest, TokenErrorMessageEOFWithContext) { + const std::string text("hello, world\nbye w0rld (\n"); + const TokenInfo error_token(TokenInfo::EOFToken()); + FakeFileAnalyzer analyzer(text, "unbalanced.txt"); + { + const auto message = analyzer.TokenErrorMessage(error_token); + EXPECT_EQ(message, "token: <> at 3:1"); + } + { + constexpr bool with_diagnostic_contex = true; + const auto message = analyzer.LinterTokenErrorMessage( + {error_token, AnalysisPhase::kParsePhase}, with_diagnostic_contex); + EXPECT_TRUE(absl::StrContains( + message, "unbalanced.txt:3:1: syntax error (unexpected EOF)")); + } +} + } // namespace } // namespace verible diff --git a/verilog/analysis/verilog_analyzer_test.cc b/verilog/analysis/verilog_analyzer_test.cc index 225ca376b..1f737b8d6 100644 --- a/verilog/analysis/verilog_analyzer_test.cc +++ b/verilog/analysis/verilog_analyzer_test.cc @@ -74,10 +74,10 @@ bool TreeContainsToken(const ConcreteSyntaxTree& tree, const TokenInfo& token) { } void DiagnosticMessagesContainFilename(const VerilogAnalyzer& analyzer, - absl::string_view filename) { - constexpr bool with_diagnostic_contex = false; + absl::string_view filename, + bool with_diagnostic_context) { const std::vector syntax_error_messages( - analyzer.LinterTokenErrorMessages(with_diagnostic_contex)); + analyzer.LinterTokenErrorMessages(with_diagnostic_context)); for (const auto& message : syntax_error_messages) { EXPECT_TRUE(absl::StrContains(message, filename)); } @@ -103,7 +103,8 @@ TEST(AnalyzeVerilogLexerTest, RejectsBadId) { const auto& rejects = analyzer_ptr->GetRejectedTokens(); EXPECT_THAT(rejects, SizeIs(1)); EXPECT_EQ(rejects.front().phase, AnalysisPhase::kLexPhase); - DiagnosticMessagesContainFilename(*analyzer_ptr, ""); + DiagnosticMessagesContainFilename(*analyzer_ptr, "", false); + DiagnosticMessagesContainFilename(*analyzer_ptr, "", true); } // Tests that invalid macro identifier is rejected. @@ -116,7 +117,8 @@ TEST(AnalyzeVerilogLexerTest, RejectsMacroBadId) { const auto& rejects = analyzer_ptr->GetRejectedTokens(); EXPECT_THAT(rejects, SizeIs(1)); EXPECT_EQ(rejects.front().phase, AnalysisPhase::kLexPhase); - DiagnosticMessagesContainFilename(*analyzer_ptr, ""); + DiagnosticMessagesContainFilename(*analyzer_ptr, "", false); + DiagnosticMessagesContainFilename(*analyzer_ptr, "", true); } // The following tests check that standalone Verilog expression parsing work. @@ -160,7 +162,8 @@ TEST(AnalyzeVerilogExpressionTest, ParsesUnfinishedOp) { const auto& rejects = analyzer_ptr->GetRejectedTokens(); EXPECT_THAT(rejects, SizeIs(1)); EXPECT_EQ(rejects.front().phase, AnalysisPhase::kParsePhase); - DiagnosticMessagesContainFilename(*analyzer_ptr, ""); + DiagnosticMessagesContainFilename(*analyzer_ptr, "", false); + DiagnosticMessagesContainFilename(*analyzer_ptr, "", true); } TEST(AnalyzeVerilogExpressionTest, Unbalanced) { @@ -171,7 +174,8 @@ TEST(AnalyzeVerilogExpressionTest, Unbalanced) { const auto& rejects = analyzer_ptr->GetRejectedTokens(); EXPECT_THAT(rejects, SizeIs(1)); EXPECT_EQ(rejects.front().phase, AnalysisPhase::kParsePhase); - DiagnosticMessagesContainFilename(*analyzer_ptr, ""); + DiagnosticMessagesContainFilename(*analyzer_ptr, "", false); + DiagnosticMessagesContainFilename(*analyzer_ptr, "", true); } TEST(AnalyzeVerilogExpressionTest, ParsesConcat) { @@ -208,7 +212,8 @@ TEST(AnalyzeVerilogExpressionTest, RejectsModuleItemAttack) { << "got: [\n" << verible::SequenceFormatter(rejects, "\n") << "\n]"; EXPECT_EQ(rejects.front().phase, AnalysisPhase::kParsePhase); - DiagnosticMessagesContainFilename(*analyzer_ptr, ""); + DiagnosticMessagesContainFilename(*analyzer_ptr, "", false); + DiagnosticMessagesContainFilename(*analyzer_ptr, "", true); } // The following tests check that standalone Verilog module-body parsing works. @@ -285,7 +290,8 @@ TEST(AnalyzeVerilogClassBodyTest, RejectsModuleItem) { const auto& first_reject = rejects.front(); EXPECT_EQ(first_reject.phase, AnalysisPhase::kParsePhase); EXPECT_EQ(first_reject.token_info.text(), "initial"); - DiagnosticMessagesContainFilename(*analyzer_ptr, ""); + DiagnosticMessagesContainFilename(*analyzer_ptr, "", false); + DiagnosticMessagesContainFilename(*analyzer_ptr, "", true); } // The following tests check for package-body parsing. @@ -315,7 +321,8 @@ TEST(AnalyzeVerilogClassBodyTest, RejectsWireDeclaration) { const auto& rejects = analyzer_ptr->GetRejectedTokens(); EXPECT_THAT(rejects, SizeIs(1)); EXPECT_EQ(rejects.front().phase, AnalysisPhase::kParsePhase); - DiagnosticMessagesContainFilename(*analyzer_ptr, ""); + DiagnosticMessagesContainFilename(*analyzer_ptr, "", false); + DiagnosticMessagesContainFilename(*analyzer_ptr, "", true); } TEST(AnalyzeVerilogLibraryMapTest, ParsesLibraryDeclaration) { @@ -386,7 +393,8 @@ TEST(AnalyzeVerilogAutomaticMode, ModuleBodyModeSyntaxError) { filename); const auto status = ABSL_DIE_IF_NULL(analyzer_ptr)->ParseStatus(); EXPECT_FALSE(status.ok()); - DiagnosticMessagesContainFilename(*analyzer_ptr, filename); + DiagnosticMessagesContainFilename(*analyzer_ptr, filename, false); + DiagnosticMessagesContainFilename(*analyzer_ptr, filename, true); } TEST(AnalyzeVerilogAutomaticMode, ClassBodyMode) { diff --git a/verilog/analysis/verilog_linter_test.cc b/verilog/analysis/verilog_linter_test.cc index 09f6fbf56..cbd16f0bf 100644 --- a/verilog/analysis/verilog_linter_test.cc +++ b/verilog/analysis/verilog_linter_test.cc @@ -178,12 +178,19 @@ class VerilogLinterTest : public DefaultLinterConfigTestFixture, const absl::Status status = ABSL_DIE_IF_NULL(analyzer)->Analyze(); std::ostringstream diagnostics; if (!status.ok()) { - constexpr bool with_diagnostic_contex = false; + bool with_diagnostic_contex = false; const std::vector syntax_error_messages( analyzer->LinterTokenErrorMessages(with_diagnostic_contex)); for (const auto& message : syntax_error_messages) { diagnostics << message << std::endl; } + + with_diagnostic_contex = true; + const std::vector syntax_error_messages_with_context( + analyzer->LinterTokenErrorMessages(with_diagnostic_contex)); + for (const auto& message : syntax_error_messages_with_context) { + diagnostics << message << std::endl; + } } // For testing purposes we want the status returned to reflect // lint success, so as long as we have a syntax tree (even if there From a90bd77b130d175b96fe446cde78901688b1f5cd Mon Sep 17 00:00:00 2001 From: Pawel Sagan Date: Fri, 5 Mar 2021 21:03:29 +0100 Subject: [PATCH 5/8] fixing typos in variable names --- common/analysis/file_analyzer_test.cc | 32 +++++++++++------------ verilog/formatting/tree_unwrapper_test.cc | 4 +-- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/common/analysis/file_analyzer_test.cc b/common/analysis/file_analyzer_test.cc index 0c138651f..762a03af6 100644 --- a/common/analysis/file_analyzer_test.cc +++ b/common/analysis/file_analyzer_test.cc @@ -63,9 +63,9 @@ TEST(FileAnalyzerTest, TokenErrorMessageSameLine) { EXPECT_EQ(message, "token: \"w0rld\" at 2:5-9"); } { - constexpr bool with_diagnostic_contex = false; + constexpr bool with_diagnostic_context = false; const auto message = analyzer.LinterTokenErrorMessage( - {error_token, AnalysisPhase::kParsePhase}, with_diagnostic_contex); + {error_token, AnalysisPhase::kParsePhase}, with_diagnostic_context); EXPECT_TRUE(absl::StrContains( message, "hello.txt:2:5: syntax error, rejected \"w0rld\"")); } @@ -81,9 +81,9 @@ TEST(FileAnalyzerTest, TokenErrorMessageSameLineWithContext) { EXPECT_EQ(message, "token: \"w0rld\" at 2:5-9"); } { - constexpr bool with_diagnostic_contex = true; + constexpr bool with_diagnostic_context = true; const auto message = analyzer.LinterTokenErrorMessage( - {error_token, AnalysisPhase::kParsePhase}, with_diagnostic_contex); + {error_token, AnalysisPhase::kParsePhase}, with_diagnostic_context); EXPECT_TRUE( absl::StrContains(message, "hello.txt:2:5: syntax error, rejected \"w0rld\" " @@ -101,9 +101,9 @@ TEST(FileAnalyzerTest, TokenErrorMessageOneChar) { EXPECT_EQ(message, "token: \",\" at 1:6"); } { - constexpr bool with_diagnostic_contex = false; + constexpr bool with_diagnostic_context = false; const auto message = analyzer.LinterTokenErrorMessage( - {error_token, AnalysisPhase::kParsePhase}, with_diagnostic_contex); + {error_token, AnalysisPhase::kParsePhase}, with_diagnostic_context); EXPECT_TRUE(absl::StrContains( message, "hello.txt:1:6: syntax error, rejected \",\"")); } @@ -119,9 +119,9 @@ TEST(FileAnalyzerTest, TokenErrorMessageOneCharWithContext) { EXPECT_EQ(message, "token: \",\" at 1:6"); } { - constexpr bool with_diagnostic_contex = true; + constexpr bool with_diagnostic_context = true; const auto message = analyzer.LinterTokenErrorMessage( - {error_token, AnalysisPhase::kParsePhase}, with_diagnostic_contex); + {error_token, AnalysisPhase::kParsePhase}, with_diagnostic_context); EXPECT_TRUE(absl::StrContains(message, "hello.txt:1:6: syntax error, rejected \",\" " "(syntax-error).\nhello, world\n ^")); @@ -138,9 +138,9 @@ TEST(FileAnalyzerTest, TokenErrorMessageDifferentLine) { EXPECT_EQ(message, "token: \"world\nbye\" at 1:8-2:3"); } { - constexpr bool with_diagnostic_contex = false; + constexpr bool with_diagnostic_context = false; const auto message = analyzer.LinterTokenErrorMessage( - {error_token, AnalysisPhase::kParsePhase}, with_diagnostic_contex); + {error_token, AnalysisPhase::kParsePhase}, with_diagnostic_context); EXPECT_TRUE(absl::StrContains( message, "hello.txt:1:8: syntax error, rejected \"world\nbye\"")); } @@ -156,9 +156,9 @@ TEST(FileAnalyzerTest, TokenErrorMessageDifferentLineWithContext) { EXPECT_EQ(message, "token: \"world\nbye\" at 1:8-2:3"); } { - constexpr bool with_diagnostic_contex = true; + constexpr bool with_diagnostic_context = true; const auto message = analyzer.LinterTokenErrorMessage( - {error_token, AnalysisPhase::kParsePhase}, with_diagnostic_contex); + {error_token, AnalysisPhase::kParsePhase}, with_diagnostic_context); EXPECT_TRUE(absl::StrContains( message, "hello.txt:1:8: syntax error, rejected \"world\nbye\" " @@ -177,9 +177,9 @@ TEST(FileAnalyzerTest, TokenErrorMessageEOF) { EXPECT_EQ(message, "token: <> at 3:1"); } { - constexpr bool with_diagnostic_contex = false; + constexpr bool with_diagnostic_context = false; const auto message = analyzer.LinterTokenErrorMessage( - {error_token, AnalysisPhase::kParsePhase}, with_diagnostic_contex); + {error_token, AnalysisPhase::kParsePhase}, with_diagnostic_context); EXPECT_TRUE(absl::StrContains( message, "unbalanced.txt:3:1: syntax error (unexpected EOF)")); } @@ -196,9 +196,9 @@ TEST(FileAnalyzerTest, TokenErrorMessageEOFWithContext) { EXPECT_EQ(message, "token: <> at 3:1"); } { - constexpr bool with_diagnostic_contex = true; + constexpr bool with_diagnostic_context = true; const auto message = analyzer.LinterTokenErrorMessage( - {error_token, AnalysisPhase::kParsePhase}, with_diagnostic_contex); + {error_token, AnalysisPhase::kParsePhase}, with_diagnostic_context); EXPECT_TRUE(absl::StrContains( message, "unbalanced.txt:3:1: syntax error (unexpected EOF)")); } diff --git a/verilog/formatting/tree_unwrapper_test.cc b/verilog/formatting/tree_unwrapper_test.cc index 21f66d612..f1847efe9 100644 --- a/verilog/formatting/tree_unwrapper_test.cc +++ b/verilog/formatting/tree_unwrapper_test.cc @@ -239,9 +239,9 @@ class TreeUnwrapperTest : public ::testing::Test { // Since source code is required to be valid, this error-handling is just // to help debug the test case construction if (!status.ok()) { - constexpr bool with_diagnostic_contex = false; + constexpr bool with_diagnostic_context = false; const std::vector syntax_error_messages( - analyzer_->LinterTokenErrorMessages(with_diagnostic_contex)); + analyzer_->LinterTokenErrorMessages(with_diagnostic_context)); for (const auto& message : syntax_error_messages) { std::cout << message << std::endl; } From aa0d9e7cf8b167793fedf6a4941276519a25a7dd Mon Sep 17 00:00:00 2001 From: Pawel Sagan Date: Thu, 18 Mar 2021 14:58:02 +0100 Subject: [PATCH 6/8] formating with clang --- common/analysis/file_analyzer.cc | 11 ++++++----- verilog/tools/syntax/verilog_syntax.cc | 6 ++++-- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/common/analysis/file_analyzer.cc b/common/analysis/file_analyzer.cc index 7218c5a0e..f4d9223be 100644 --- a/common/analysis/file_analyzer.cc +++ b/common/analysis/file_analyzer.cc @@ -139,15 +139,14 @@ std::string FileAnalyzer::LinterTokenErrorMessage( const RejectedToken& error_token, bool diagnostic_context) const { const LineColumnMap& line_column_map = Data().GetLineColumnMap(); const absl::string_view base_text = Data().Contents(); - + std::ostringstream output_stream; output_stream << filename_ << ':'; if (!error_token.token_info.isEOF()) { const auto left = line_column_map(error_token.token_info.left(base_text)); output_stream << left << ": " << error_token.phase << " error, rejected \"" << error_token.token_info.text() << "\" (syntax-error)."; - if(diagnostic_context) - { + if (diagnostic_context) { const auto& lines = Data().Lines(); if (left.line < static_cast(lines.size())) { output_stream << "\n" << lines[left.line] << std::endl; @@ -167,11 +166,13 @@ std::string FileAnalyzer::LinterTokenErrorMessage( return output_stream.str(); } -std::vector FileAnalyzer::LinterTokenErrorMessages(bool diagnostic_context) const { +std::vector FileAnalyzer::LinterTokenErrorMessages( + bool diagnostic_context) const { std::vector messages; messages.reserve(rejected_tokens_.size()); for (const auto& rejected_token : rejected_tokens_) { - messages.push_back(LinterTokenErrorMessage(rejected_token, diagnostic_context)); + messages.push_back( + LinterTokenErrorMessage(rejected_token, diagnostic_context)); } return messages; } diff --git a/verilog/tools/syntax/verilog_syntax.cc b/verilog/tools/syntax/verilog_syntax.cc index 99cf1e2f1..fb0c0105a 100644 --- a/verilog/tools/syntax/verilog_syntax.cc +++ b/verilog/tools/syntax/verilog_syntax.cc @@ -157,12 +157,14 @@ static int AnalyzeOneFile(absl::string_view content, absl::string_view filename, if (!lex_status.ok() || !parse_status.ok()) { const std::vector syntax_error_messages( - analyzer->LinterTokenErrorMessages(absl::GetFlag(FLAGS_show_diagnostic_context))); + analyzer->LinterTokenErrorMessages( + absl::GetFlag(FLAGS_show_diagnostic_context))); const int error_limit = absl::GetFlag(FLAGS_error_limit); int error_count = 0; if (!absl::GetFlag(FLAGS_export_json)) { const std::vector syntax_error_messages( - analyzer->LinterTokenErrorMessages()); + analyzer->LinterTokenErrorMessages( + absl::GetFlag(FLAGS_show_diagnostic_context))); for (const auto& message : syntax_error_messages) { std::cout << message << std::endl; ++error_count; From c292195ca5e0fca6757669f15f047ea29383d4cf Mon Sep 17 00:00:00 2001 From: Pawel Sagan Date: Thu, 18 Mar 2021 16:59:14 +0100 Subject: [PATCH 7/8] adding shell tests for diagnostic context check, passing valid option to verilog linter --- verilog/analysis/verilog_linter.cc | 3 +-- verilog/tools/lint/lint_tool_test.sh | 29 ++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/verilog/analysis/verilog_linter.cc b/verilog/analysis/verilog_linter.cc index a688cbcc7..2f4e63400 100644 --- a/verilog/analysis/verilog_linter.cc +++ b/verilog/analysis/verilog_linter.cc @@ -100,9 +100,8 @@ int LintOneFile(std::ostream* stream, absl::string_view filename, const auto lex_status = ABSL_DIE_IF_NULL(analyzer)->LexStatus(); const auto parse_status = analyzer->ParseStatus(); if (!lex_status.ok() || !parse_status.ok()) { - constexpr bool with_diagnostic_context = false; const std::vector syntax_error_messages( - analyzer->LinterTokenErrorMessages(with_diagnostic_context)); + analyzer->LinterTokenErrorMessages(show_context)); for (const auto& message : syntax_error_messages) { *stream << message << std::endl; } diff --git a/verilog/tools/lint/lint_tool_test.sh b/verilog/tools/lint/lint_tool_test.sh index c23708057..14209132e 100755 --- a/verilog/tools/lint/lint_tool_test.sh +++ b/verilog/tools/lint/lint_tool_test.sh @@ -105,6 +105,35 @@ status="$?" exit 1 } +DIAGNOSTIC_OUTPUT="${TEST_TMPDIR}/expected-diagnostic.out" +cat > "${DIAGNOSTIC_OUTPUT}" < "${MY_OUTPUT_FILE}.out" + +diff "${DIAGNOSTIC_OUTPUT}" "${MY_OUTPUT_FILE}.out" +status="$?" +[[ $status == 0 ]] || { + echo "Expected exit code 0, but got $status" + exit 1 +} + +cat > "${DIAGNOSTIC_OUTPUT}" < "${MY_OUTPUT_FILE}.out" + +diff "${DIAGNOSTIC_OUTPUT}" "${MY_OUTPUT_FILE}.out" +status="$?" +[[ $status == 0 ]] || { + echo "Expected exit code 0, but got $status" + exit 1 +} + echo "=== Test --parse_fatal" "$lint_tool" "$TEST_FILE" --parse_fatal > /dev/null 2> "${MY_OUTPUT_FILE}.err" From 31a5ecd1577cadaf640c17a39b14c42e93e49525 Mon Sep 17 00:00:00 2001 From: Pawel Sagan Date: Thu, 18 Mar 2021 18:21:48 +0100 Subject: [PATCH 8/8] adding comment that exaplaint argument in LinterTokenErrorMessage function, breaking text inside tests resposible for checking the new diagnostic option --- common/analysis/file_analyzer.h | 6 ++++++ common/analysis/file_analyzer_test.cc | 12 +++++++++--- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/common/analysis/file_analyzer.h b/common/analysis/file_analyzer.h index faedfa57b..a4b935f36 100644 --- a/common/analysis/file_analyzer.h +++ b/common/analysis/file_analyzer.h @@ -94,8 +94,14 @@ class FileAnalyzer : public TextStructure { std::vector TokenErrorMessages() const; // Diagnostic message for rejected tokens for linter. + // Second argument is the show_context option. When enabled + // additional diagnostic line is concatenated to an error message + // with marker that points to vulnerable token std::string LinterTokenErrorMessage(const RejectedToken&, bool) const; + // First argument is the show_context option. When enabled + // additional diagnostic line is concatenated to an error message + // with marker that points to vulnerable token std::vector LinterTokenErrorMessages(bool) const; const std::vector& GetRejectedTokens() const { diff --git a/common/analysis/file_analyzer_test.cc b/common/analysis/file_analyzer_test.cc index 762a03af6..f1b666728 100644 --- a/common/analysis/file_analyzer_test.cc +++ b/common/analysis/file_analyzer_test.cc @@ -87,7 +87,9 @@ TEST(FileAnalyzerTest, TokenErrorMessageSameLineWithContext) { EXPECT_TRUE( absl::StrContains(message, "hello.txt:2:5: syntax error, rejected \"w0rld\" " - "(syntax-error).\nbye w0rld\n ^")); + "(syntax-error).\n" + "bye w0rld\n" + " ^")); } } @@ -124,7 +126,9 @@ TEST(FileAnalyzerTest, TokenErrorMessageOneCharWithContext) { {error_token, AnalysisPhase::kParsePhase}, with_diagnostic_context); EXPECT_TRUE(absl::StrContains(message, "hello.txt:1:6: syntax error, rejected \",\" " - "(syntax-error).\nhello, world\n ^")); + "(syntax-error).\n" + "hello, world\n" + " ^")); } } @@ -162,7 +166,9 @@ TEST(FileAnalyzerTest, TokenErrorMessageDifferentLineWithContext) { EXPECT_TRUE(absl::StrContains( message, "hello.txt:1:8: syntax error, rejected \"world\nbye\" " - "(syntax-error).\nhello, world\n ^")); + "(syntax-error).\n" + "hello, world\n" + " ^")); } }